-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
Collect active query jobs into struct QueryJobMap
#152514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| let query_map = match qcx.collect_active_jobs_from_all_queries(false) { | ||
| Ok(query_map) => query_map, | ||
| Err(query_map) => query_map, | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result<T, T> is always a weird type...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ControlFlow actually has a method for this (https://doc.rust-lang.org/stable/std/ops/enum.ControlFlow.html#method.into_value), but Result does not.
| ) -> Option<()> | ||
| ] = &[ | ||
| $(query_impl::$name::gather_active_jobs),* | ||
| $( $crate::query_impl::$name::gather_active_jobs ),* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does crate work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The macro is only ever expanded in its own crate, so $crate and crate both work and happen to mean the same thing.
When both styles are possible, I usually prefer $crate to reassure the reader that the definition can be found in the same crate as the macro, whereas crate in a macro is unusual and asks the reader to consider the possibility of it referring to a different crate from the macro.
(An argument could be made that crate is the more “logically correct” choice, because query_impl is declared by the macro at its expansion site, but since both crates are the same in practice I think the reassuring “same crate” signal is more helpful.)
| map: &'a QueryMap<'tcx>, | ||
| ) -> Option<&'a QueryLatch<'tcx>> { | ||
| map.get(&id).unwrap().job.latch.as_ref() | ||
| fn frame_of(&self, id: QueryJobId) -> &QueryStackFrame<QueryStackDeferred<'tcx>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These query_job_id_* functions were a clumsy leftover from one of my earlier PRs. This is a nice tidying up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I believe they were inherent methods of QueryJobId at the time.
|
r=me after the comments are considered. |
compiler/rustc_query_impl/src/job.rs
Outdated
| ) -> Option<&'a QueryLatch<'tcx>> { | ||
| map.get(&id).unwrap().job.latch.as_ref() | ||
| fn frame_of(&self, id: QueryJobId) -> &QueryStackFrame<QueryStackDeferred<'tcx>> { | ||
| &self.map.get(&id).unwrap().frame |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| &self.map.get(&id).unwrap().frame | |
| &self.map[&id].frame |
?
And in other cases too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adjusted the occurrences in inherent methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's more in find_cycle_in_stack and find_dep_kind_root.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was less sure about what to do with those. They're in the same file, so I guess job_map.map[&id] would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, TIL you can use [] to infallibly access a hashmap.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
@bors r+ |
Collect active query jobs into struct `QueryJobMap` This PR encapsulates the existing `QueryMap` type alias into a proper struct named `QueryJobMap`, which is used by code that wants to inspect the full set of currently-active query jobs. Wrapping the query job map in a struct makes it easier to see how other code interacts with the map, and also lets us change some free functions for map lookup into methods. We also do a renaming pass to consistently refer to the query job map as `job_map`, or as `job_map_out` in the places where it's a mutable out-parameter. There should be no change to compiler behaviour. r? nnethercote (or compiler)
Rollup of 18 pull requests Successful merges: - #150551 (Compute localized outlives constraints lazily) - #150752 (Update libc to v0.2.181) - #150988 (Improve code suggestion for incorrect macro_rules! usage) - #152422 (Change query proc macro to be more rust-analyzer friendly) - #152496 (Fix multi-cgu+debug builds using autodiff by delaying autodiff till lto) - #152514 (Collect active query jobs into struct `QueryJobMap`) - #152520 (Don't use `DepContext` in `rustc_middle::traits::cache`) - #152528 (Support serializing CodegenContext) - #152082 (Move tests) - #152232 (Add must_use for FileTimes) - #152329 (Simplify parallel! macro) - #152444 (`-Znext-solver` Prevent committing unfulfilled unsized coercion) - #152486 (remove redundant backchain attribute in codegen) - #152519 (Fix feature gating for new `try bikeshed` expressions) - #152529 (sparc64: enable abi compatibility test) - #152548 (reject inline const patterns pre-expansion) - #152550 (Port #[prelude_import] to the attribute parser) - #152552 (Add 2048-bit HvxVectorPair support to Hexagon SIMD ABI checks)
Rollup of 17 pull requests Successful merges: - #150551 (Compute localized outlives constraints lazily) - #150988 (Improve code suggestion for incorrect macro_rules! usage) - #152422 (Change query proc macro to be more rust-analyzer friendly) - #152496 (Fix multi-cgu+debug builds using autodiff by delaying autodiff till lto) - #152514 (Collect active query jobs into struct `QueryJobMap`) - #152520 (Don't use `DepContext` in `rustc_middle::traits::cache`) - #152528 (Support serializing CodegenContext) - #152082 (Move tests) - #152232 (Add must_use for FileTimes) - #152329 (Simplify parallel! macro) - #152444 (`-Znext-solver` Prevent committing unfulfilled unsized coercion) - #152486 (remove redundant backchain attribute in codegen) - #152519 (Fix feature gating for new `try bikeshed` expressions) - #152529 (sparc64: enable abi compatibility test) - #152548 (reject inline const patterns pre-expansion) - #152550 (Port #[prelude_import] to the attribute parser) - #152552 (Add 2048-bit HvxVectorPair support to Hexagon SIMD ABI checks) Failed merges: - #152515 (Extract `DepKindVTable` constructors to their own module)
Rollup merge of #152514 - Zalathar:job-map, r=nnethercote Collect active query jobs into struct `QueryJobMap` This PR encapsulates the existing `QueryMap` type alias into a proper struct named `QueryJobMap`, which is used by code that wants to inspect the full set of currently-active query jobs. Wrapping the query job map in a struct makes it easier to see how other code interacts with the map, and also lets us change some free functions for map lookup into methods. We also do a renaming pass to consistently refer to the query job map as `job_map`, or as `job_map_out` in the places where it's a mutable out-parameter. There should be no change to compiler behaviour. r? nnethercote (or compiler)
This PR encapsulates the existing
QueryMaptype alias into a proper struct namedQueryJobMap, which is used by code that wants to inspect the full set of currently-active query jobs.Wrapping the query job map in a struct makes it easier to see how other code interacts with the map, and also lets us change some free functions for map lookup into methods.
We also do a renaming pass to consistently refer to the query job map as
job_map, or asjob_map_outin the places where it's a mutable out-parameter.There should be no change to compiler behaviour.
r? nnethercote (or compiler)