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 |
rustbot
added
A-attributes
| } | ||
|
|
||
| #[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
There was a problem hiding this comment.
This
first_item_in_crateparameter 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.
so i did just that. it improved the oracles a lot and allowed me to completely remove the ugly first_item_in_crate which is good news!
but also, the code is far from perfect. i tried to cut corners where possible in order to keep the code readable. i can probably add support for nested multiline comments without making the code too hard to review, but i don't think there's anything i can realistically do to properly support the attributes (that would require keeping a stack of the opened delimiters, which in turn would requires a lexer).
i took the liberty of using try_blocks in order to keep the code somewhat concise (cool feature, btw!). it looks like this feature is already used in the compiler, so i figured it wouldn't hurt, but i can totally remove it if needed.
@rustbot ready
scrabsha
force-pushed
the
sasha/drop-derive-helpers
branch
from
38f62ee to
676a6d9
Compare
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
scrabsha
force-pushed
the
sasha/drop-derive-helpers
branch
from
676a6d9 to
88e1141
Compare
rustbot
added
S-waiting-on-review
scrabsha
force-pushed
the
sasha/drop-derive-helpers
branch
from
88e1141 to
3b5896d
Compare
This comment has been minimized.
This comment has been minimized.
scrabsha
force-pushed
the
sasha/drop-derive-helpers
branch
from
3b5896d to
0dfbffd
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. |
4 participants
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 i doubt there's much we can do to avoid that.
r? @jdonszelmann
r? @JonathanBrouwer