Clarify aspects of query macros#152779
Conversation
|
|
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…r=<try> Clarify aspects of query macros
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (e5069e4): 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 6.8%, secondary -5.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -2.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 480.396s -> 483.06s (0.55%) |
f842e54 to
8738c48
Compare
|
I have updated. I addressed most of the comments; two of them I left unresolved that I don't think warrant changes. |
|
My sticking point is applying I did find a way to simplify |
|
Ok, can you tell me which commits I should remove? |
8738c48 to
a6ed44d
Compare
This comment has been minimized.
This comment has been minimized.
|
I have updated. The commits relating to |
This comment has been minimized.
This comment has been minimized.
There are six small macros used in `define_callbacks` that all expand to one thing if a particular query modifier is present, and to another thing otherwise. One of these macros looks for the `arena_cache` modifier: - `query_if_arena` Two of these macros look for the `return_result_from_ensure_ok` modifier: - `query_ensure_select` - `ensure_ok_result` Three of these macros look for the `separate_provide_extern` modifier: - `local_key_if_separate_extern` - `separate_provide_extern_decl` - `separate_provide_extern_default` (There is also `query_helper_param_ty!`, but it follows a different pattern, and I will deal with it later.) The "one thing"/"another thing" is different for every macro. For most of them you can't see at the macro call site what the expansion will be; you have to look at the macro definition. This is annoying. This commit reduces the six macros into three macros: - `if_arena_cache` - `if_return_result_from_ensure_ok` - `if_separate_provide_extern` They all have the same form: they look for a modifier and then expand to one *given* thing or the other *given* thing. You can see at the call site the two possible expansions, which is much nicer. (Note: `query_if_arena` already had this form.) Ideally there would be a single macro instead of three, but I couldn't work out a way to combine them.
So that all `$( ... $name ... )*` repetitions are spread across multiple lines, even if they all fit on one line. I find this much easier to read because the `$name` repetition is the main feature of these macros.
It doesn't make much sense to put the query doc comments on these structs. (The doc comments *are* put on various query functions, where it makes more sense.)
Within `define_callbacks!`: - Use `Key<'tcx>` where possible instead of `$($K)*`. - Use `Value<'tcx>` where possible instead of `$V`. In the other `define_*!` macros: - Use `$K:ty` where possible instead of `$($K:tt)*`. - Add comments about unused `$K` and `$V` cases. - Add `()` key types to the special nodes in the `define_dep_nodes!` invocation for consistency with normal queries.
a6ed44d to
bc1b6b8
Compare
|
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 have rebased. I know I have a lot of query PRs in flight right now. If I had a choice I would get this one merged first, because the reindenting in the second commit is conflict-prone. |
Various clarity and consistency improvements to query macros.
r? @Zalathar