-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Remove NtIdent
and NtLifetime
.
#96627
Conversation
There's no need to use Interpolated for these because they both consist of a single token, and so there's no operator precedence problems with inserting them directly into the token stream.
This is an experiment. The compiler successfully bootstraps. There are 21 ui test failures, all involving error messages where the spans have changed. I did a bunch of fiddling but was unable to improve things. Going from an |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 115f10b with merge 542ef89b32e7c07bdcd6582867c64b8e36096e60... |
The job Click to see the possible cause of the failure (guessed by this bot)
|
☀️ Try build successful - checks-actions |
Queued 542ef89b32e7c07bdcd6582867c64b8e36096e60 with parent f6cbc92, future comparison URL. |
Finished benchmarking commit (542ef89b32e7c07bdcd6582867c64b8e36096e60): comparison url. Summary:
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
|
I planned to keep two spans in each token - #95580. |
Interesting. In that PR you just called the second one |
Not sure, |
(Re: the spans only:) I'd like to see what the output difference is for the errors: I might ok a change like this if the fallout isn't terrible. |
The fallout looks about what I expected, and in some cases it isn't materially worse, but there are cases like https://github.com/rust-lang/rust/runs/6253335801?check_suite_focus=true#step:26:2583 and https://github.com/rust-lang/rust/runs/6253335801?check_suite_focus=true#step:26:2375 that are problematic. I usually create a new commit with the |
I tried to make changes in this area in the past, including removal of these additional spans, and such removal really messes up spans in macros, anything including things like So I never considered it acceptable to just remove these spans, despite that keeping them caused some noticeable headaches. |
This won't work in its current form. |
There's no need to use Interpolated for these because they both consist
of a single token, and so there's no operator precedence problems with
inserting them directly into the token stream.
r? @ghost