-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Some rustc_query_system cleanups
#152023
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
base: main
Are you sure you want to change the base?
Some rustc_query_system cleanups
#152023
Conversation
| @@ -7,7 +7,6 @@ use rustc_errors::codes::*; | |||
| use rustc_errors::{Applicability, MultiSpan, pluralize, struct_span_code_err}; | |||
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.
This module also belongs under the query module, if you feel like moving it.
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.
Do you mean that rustc_middle::values should be rustc_middle::query::values?
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.
Yes
This comment has been minimized.
This comment has been minimized.
| } | ||
| } | ||
|
|
||
| impl<Key, Value> Cache<Key, Value> { |
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.
For future work, note that this Cache type appears to only be used by trait selection/evaluation, and has nothing to do with query caching at all!
It should probably be moved into rustc_middle::traits.
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 have a draft change doing exactly that, and I even considered including it in this PR!
I think the reason it's in rustc_query_system is because it uses WithDepNode, and a lot of depnode-related stuff is in that crate. But I think it makes sense to place things more by where they are used (in rustc_middle) rather than by conceptual categorization.
I'll include it in a future PR :)
|
r=me with Zoxc's comment resolved |
| query_system_query_overflow = queries overflow the depth limit! | ||
| .help = consider increasing the recursion limit by adding a `#![recursion_limit = "{$suggested_limit}"]` attribute to your crate (`{$crate_name}`) | ||
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 whole file is removed in #152041.
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.
Ok, I'll wait for that to merge.
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.
Thanks, that was helpful to know about that in advance before rebasing.
It's a tiny module with one trait and a default impl. It's not used in `rustc_query_system`; all uses and non-default impls are in `rustc_middle` and `rustc_query_impl`. This commit moves it into `rustc_middle`, which makes things simpler overall.
It's a better place for it, because it relates to queries.
They are defined in `rustc_query_system` but used in `rustc_query_impl`. This is very much *not* how things are supposed to be done; I suspect someone got lazy and took a shortcut at some point. This commit moves the errors into `rustc_query_impl`. This requires more lines of code to give `rustc_query_impl` an errors module, but it's worthwhile to do things in the normal way instead of a weird exceptional way.
It's unused. And it's nice to remove this function that didn't behave like normal `clear` does, as the comment explained.
|
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. |
|
I rebased over #152041, and added a new commit to move |
|
@bors r=cjgillot |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
Small improvements I found while looking closely at
rustc_query_system. Best reviewed one commit at a time.r? @cjgillot