-
Notifications
You must be signed in to change notification settings - Fork 13.8k
Extract most code from define_feedable!
#147393
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: master
Are you sure you want to change the base?
Conversation
define_feedable!
No perf changes intended, but let's check: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
Extract most code from `define_feedable!`
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
} | ||
|
||
/// Common implementation of query feeding, used by `define_feedable!`. | ||
pub(crate) fn query_feed_inner<'tcx, Cache, 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.
Could you put this closer to query_get_at
and co? It serves a similar purpose.
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.
Hmm, the downside would be that it's no longer adjacent to the calling code in define_feedable!
, so coordinated changes to both would be much less convenient.
I can move it if you want, but to me it seems better off here.
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.
Another approach would be to move the entire family of query-method-inner functions into their own file.
That would allow them to be kept together, without getting lost in a sea of boilerplate macros in query::plumbing
.
/// Common implementation of query feeding, used by `define_feedable!`. | ||
pub(crate) fn query_feed_inner<'tcx, Cache, Value>( | ||
tcx: TyCtxt<'tcx>, | ||
name: &'static str, |
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 should be recoverable from dep_kind
in a panic message.
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.
Ah, via tcx.dep_kind_info(dep_kind).name
? That should be easy enough.
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. Or using Debug for DepKind
which uses it internally. It's for ICEs anyway.
Finished benchmarking commit (b135ecc): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results (primary -7.0%, secondary -5.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 471.633s -> 471.589s (-0.01%) |
r? @cjgillot Seems you probably are a better reviewer for this PR. |
This PR was rebased onto a different master 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. |
Thanks! |
Perf results were neutral, so this should be fine for rollup. @bors rollup=maybe |
Extract most code from `define_feedable!` This PR extracts most of the non-trivial code from the `define_feedable!` macro (which defines the `TyCtxtFeed::$query` methods), and moves it to a helper function `query_feed_inner` written in ordinary non-macro code. Doing so should make that code easier to read and modify, because it now gets proper IDE support and has explicit trait bounds. --- There should be no change in compiler behaviour. I've structured the commits so that the actual extraction part is mostly just whitespace changes, making it easier to review individually with whitespace changes hidden.
Rollup of 12 pull requests Successful merges: - #138799 (core: simplify `Extend` for tuples) - #146692 (Save x.py's help text for saving output time) - #147168 (Don't unconditionally build alloc for `no-std` targets) - #147178 ([DebugInfo] Improve formatting of MSVC enum struct variants) - #147240 (Add an ACP list item to the library tracking issue template) - #147246 (Explain not existed key in BTreeMap::split_off) - #147393 (Extract most code from `define_feedable!`) - #147495 (Update wasm-component-ld to 0.5.18) - #147503 (Fix documentation of Instant::now on mac) - #147541 (Change int-to-ptr transmute lowering back to inttoptr) - #147549 (Replace `LLVMRustContextCreate` with normal LLVM-C API calls) - #147596 (Adjust the Arm targets in CI to reflect latest changes) r? `@ghost` `@rustbot` modify labels: rollup
Extract most code from `define_feedable!` This PR extracts most of the non-trivial code from the `define_feedable!` macro (which defines the `TyCtxtFeed::$query` methods), and moves it to a helper function `query_feed_inner` written in ordinary non-macro code. Doing so should make that code easier to read and modify, because it now gets proper IDE support and has explicit trait bounds. --- There should be no change in compiler behaviour. I've structured the commits so that the actual extraction part is mostly just whitespace changes, making it easier to review individually with whitespace changes hidden.
Rollup of 8 pull requests Successful merges: - #138799 (core: simplify `Extend` for tuples) - #145897 (Rehome 30 `tests/ui/issues/` tests to other subdirectories under `tests/ui/` [#4 of Batch #2]) - #146692 (Save x.py's help text for saving output time) - #147240 (Add an ACP list item to the library tracking issue template) - #147246 (Explain not existed key in BTreeMap::split_off) - #147393 (Extract most code from `define_feedable!`) - #147503 (Fix documentation of Instant::now on mac) - #147549 (Replace `LLVMRustContextCreate` with normal LLVM-C API calls) r? `@ghost` `@rustbot` modify labels: rollup
This PR extracts most of the non-trivial code from the
define_feedable!
macro (which defines theTyCtxtFeed::$query
methods), and moves it to a helper functionquery_feed_inner
written in ordinary non-macro code.Doing so should make that code easier to read and modify, because it now gets proper IDE support and has explicit trait bounds.
There should be no change in compiler behaviour.
I've structured the commits so that the actual extraction part is mostly just whitespace changes, making it easier to review individually with whitespace changes hidden.