Skip to content

Conversation

mejrs
Copy link
Contributor

@mejrs mejrs commented Jul 1, 2025

I went back and forth on the design a bit, this is what I settled on.

r? @jdonszelmann

@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2025

jdonszelmann is not on the review rotation at the moment.
They may take a while to respond.

@rustbot rustbot added A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 1, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jul 1, 2025

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Jul 3, 2025

☔ The latest upstream changes (presumably #143363) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Collaborator

bors commented Jul 8, 2025

☔ The latest upstream changes (presumably #143645) made this pull request unmergeable. Please resolve the merge conflicts.

@jdonszelmann
Copy link
Contributor

I did look at this version, and asked @JonathanBrouwer to come up with a version that'd combine this PR and my version which I pushed to my fork a while ago. I'll compare the two implementations. That version is #145085

@mejrs
Copy link
Contributor Author

mejrs commented Aug 12, 2025

I did look at this version, and asked @JonathanBrouwer to come up with a version that'd combine this PR and my version which I pushed to my fork a while ago. I'll compare the two implementations. That version is #145085

I see. The primary motivation for my approach where the target enum references the ast of the target is that it lets us move a lot of logic from various passes (not only CheckAttr) into attribute parsing. That's what I'm missing from your and jonathan's versions.

@jdonszelmann
Copy link
Contributor

hmm, I quite like reusing the old Target, and the current version is quite declarative which I liked a lot. I'm sorry, but I'm not sure I'd want to do in this direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants