Call features_query fewer times in smart_resolve_macro_path#155155
Call features_query fewer times in smart_resolve_macro_path#155155fmease wants to merge 1 commit intorust-lang:mainfrom
features_query fewer times in smart_resolve_macro_path#155155Conversation
|
@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.
Call `features_query` fewer times in `smart_resolve_macro_path`
| .collect::<Vec<_>>(); | ||
| let typo = find_best_match_for_name(&candidates, attribute.ident.name, Some(5)) | ||
| let stable_diagnostic_attributes: Vec<_> = | ||
| [sym::on_unimplemented, sym::do_not_recommend, sym::on_move] |
There was a problem hiding this comment.
(preexisting) on_move "stable"? That's news to me. Should probably be gated as well. Or is it gated elsewhere?
There was a problem hiding this comment.
I remember that attribute being gated, so it's gate is probably elsewhere
There was a problem hiding this comment.
There was a problem hiding this comment.
Ah, thanks, I see, obvious in hindsight
There was a problem hiding this comment.
Can you give on_move the same feature check as on_const/on_unknown below? Thanks!
| (sym::on_unknown, self.tcx.features().diagnostic_on_unknown()), | ||
| ]; | ||
|
|
||
| if res == Res::NonMacroAttr(NonMacroAttrKind::Tool) |
There was a problem hiding this comment.
(preexisting) Kinda odd that we perform this check here instead of over in rustc_attr_parsing. Of course, this check has existed long before rustc_attr_parsing was introduced but now we should have better infrastructure in place.
Note that I'm not super familiar with the "low-level entry point" of the new attribute parsing infrastructure, so I don't know if there's a good reason why this check happens here.
There was a problem hiding this comment.
Yeah I still have plans to move feature gating to the new attribute parsing Infrastructure, but that is nicer to do after the lint attributes are ported again
This comment has been minimized.
This comment has been minimized.
| .collect::<Vec<_>>(); | ||
| let typo = find_best_match_for_name(&candidates, attribute.ident.name, Some(5)) | ||
| let stable_diagnostic_attributes: Vec<_> = | ||
| [sym::on_unimplemented, sym::do_not_recommend, sym::on_move] |
There was a problem hiding this comment.
Can you give on_move the same feature check as on_const/on_unknown below? Thanks!
| let typo = find_best_match_for_name( | ||
| &stable_diagnostic_attributes, | ||
| attribute.ident.name, | ||
| Some(5), |
There was a problem hiding this comment.
I just realized that on_move and on_const are really similar to each other (their edit distance is 4), so this should probably be
| Some(5), | |
| None, |
|
Finished benchmarking commit (bbece70): comparison URL. Overall result: no relevant changes - no action neededBenchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. @bors rollup=never Instruction countThis perf run didn't have relevant results for this metric. Max RSS (memory usage)Results (secondary -1.6%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 3.0%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.0%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 491.284s -> 489.863s (-0.29%) |
|
Hmm, too bad. |
|
|
|
I guess I'll repurpose this PR as a ~cleanup one ^^ once I've applied mejrs's suggestions. |
#152901 (comment)