port lint attributes - #162811
port lint attributes#162811mejrs wants to merge 6 commits into
Conversation
|
Some changes occurred in compiler/rustc_passes/src/check_attr.rs cc @jdonszelmann, @JonathanBrouwer These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in compiler/rustc_attr_ir cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer
cc @rust-lang/clippy |
| LL | let y: u32 = (x?).try_into().unwrap(); | ||
| | + +++++++++++++++++++++ |
There was a problem hiding this comment.
Uh yeah I don't know what's happening here
There was a problem hiding this comment.
I think this is minor enough that I'll put it on my todo list as something that we should fix after merging this PR
|
I'll take a look at this on Friday! Thanks for doing this! |
|
Thanks for picking this up ❤️ |
5f087e2 to
c90513f
Compare
|
cc @cjgillot @petrochenkov in case you'd like to take a look (as reviewers of the original pr #155691) |
I'll go ahead and cherrypick some changes from this PR then, some things can be split off. |
|
Blocked on #162813 |
This comment has been minimized.
This comment has been minimized.
c90513f to
5c86bdf
Compare
|
@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.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (837b948): comparison URL. Overall result: ❌✅ regressions and improvements - BENCHMARK(S) FAILEDBenchmarking 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. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf ❗ ❗ ❗ ❗ ❗
❗ ❗ ❗ ❗ ❗ Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -7.1%, secondary -1.8%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary -7.6%, secondary 2.7%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 495.925s -> 496.275s (0.07%) |
|
@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.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (49b38b0): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking 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. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -0.8%, secondary 0.5%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary 0.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeResults (primary 0.1%, secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Bootstrap: 489.814s -> 491.065s (0.26%) |
|
Ooh that's better! 🎉 |
There was a problem hiding this comment.
These are the last comments I have.
I think there's a lot of cleanup work to do after this PR, but this PR is now in a state that merging it will make the situation better rather than worse, so I'm fine with doing these in followup PRs to keep the size of this one manageable.
We should make a list of follow up items though, so they're not forgotten.
@petrochenkov I noticed that you assigned yourself a while ago, would you still like to take a look at this PR?
| LL | let y: u32 = (x?).try_into().unwrap(); | ||
| | + +++++++++++++++++++++ |
There was a problem hiding this comment.
I think this is minor enough that I'll put it on my todo list as something that we should fix after merging this PR
|
Interesting how this benchmark differs from #152369 (comment) I wonder if it has something to do with how that PR also moved all the lint store stuff to attribute parsing. |
|
I think indeed there's likely more performance to gain here, but it's already positive so we can delay that :) |
|
You did make |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (be9d55c): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking 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. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (primary -2.6%, secondary -3.2%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (primary 4.1%, secondary 4.9%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 491.424s -> 491.568s (0.03%) |
|
Perf seems slightly worse than the previous run but it's still positive, and the behavior is more correct now, so let's keep things this way |
There was a problem hiding this comment.
r=me
Thanks so much for all the effort! This has been a lot of work, both from you and @Bryntet
@petrochenkov do you still want to take a look?
@bors rollup=never
View all comments
Ports the lint attributes. I've kept this as small as possible, but there is a decent amount of clean up/refactoring that can be done afterwards,
r? @JonathanBrouwer @jdonszelmann
cc @Bryntet