drop derive helpers during attribute parsing#153540
drop derive helpers during attribute parsing#153540scrabsha wants to merge 3 commits intorust-lang:mainfrom
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
| } | ||
|
|
||
| #[derive(Copy, Clone, Debug)] | ||
| #[derive(Copy, Clone, Debug, PartialEq)] |
There was a problem hiding this comment.
You can avoid needing this impl by using matches! instead of (in)equality operators.
There was a problem hiding this comment.
i'm sorry... is there a policy for this or something? i added this because i wanted to check equality and... it sounds natural to use the == for this? i did look at the rustc dev guide before adding the derive impl this and found nothing about it.
to be clear, i'm not against using matches!() everywhere (it's already reverted), i just want to understand :/
There was a problem hiding this comment.
I quite like matches! or if-let. It avoids some codegen for the PartialEq impl too. I've also had cases where I wanted to find all usages of some type, and == isn't always obvious.
There was a problem hiding this comment.
I'm sorry... is there a policy for this or something?
There's none, it's just what I've seen other people do and what I've been doing myself. It's usually nice to avoid introducing more derives because some things have quite a lot of derives already and it's also easier on codegen. And some things, like matching on an enum variant with a field, you can't always do by calling a partialeq impl.
| allowed_targets: &AllowedTargets, | ||
| target: Target, | ||
| cx: &mut AcceptContext<'_, 'sess, S>, | ||
| first_item_in_crate: Option<Span>, |
There was a problem hiding this comment.
This first_item_in_crate parameter is rather ugly, especially because None is passed to it so often. I haven't tried but maybe you'd be better off just grabbing the span of the attribute and then going through the sourcemap looking for the next non-[comment, doc comment, other attribute etc] thing and pointing at that.
There was a problem hiding this comment.
that's a good idea, i'll experiment on this later today
@rustbot author
38f62ee to
676a6d9
Compare
|
Reminder, once the PR becomes ready for a review, use |
| | | ||
| LL | #![bench] | ||
| | ^^^^^^^^^ | ||
| ... |
There was a problem hiding this comment.
these changes are unfortunate
fixes #153102.
first two commits (7db3f6e, 0033b31) move attribute target checks from
rustc_passestorustc_attr_parsing. last commit (38f62ee) actually fixes the issue.the diagnostics slightly regressed and i'm not super happy about it, but it seemed way too complicated to maintain the "the inner attribute doesn't annotate this <item kind>" in most cases. i think that's ok, but i can try harder if y'all decide it's worth keeping it :)
r? @jdonszelmann
r? @JonathanBrouwer