-
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
Split AttrArgs
from MacArgs
.
#96209
Split AttrArgs
from MacArgs
.
#96209
Conversation
`MacArgs` is confusing. It has three variants. It's used in two different cases. - Attributes, where all three variants are used. - Function-like macro calls, where only one variant is used. This commit separates these two cases. `AttrArgs` keeps much of the functionality of the old `MacArgs`. The new `MacArgs` is a greatly streamlined version of the old `MacArgs`. There is a small amount of code duplication, but I think this change improves readability. It also lets a few unreachable code paths (e.g. `unwrap`/`expect` calls) be removed. The commit also renames `MacDelimeter` as `AttrMacDelimeter`, because it's used for both `AttrArgs` and `MacArgs`. For this type, it makes more sense to share a single type between the two cases because all variants and methods are used in both cases.
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy Some changes occurred in src/tools/rustfmt. cc @rust-lang/rustfmt |
Those two are the same case, that's why I introduced |
In what way are they the same case? I found this naming confusing. I also don't like it when types allow invalid values, e.g. if a macro call involves a |
I reviewed the changes, and I'm still against doing this. First about the terminology - Second, I'd like to keep This only leaves |
☔ The latest upstream changes (presumably #96495) made this pull request unmergeable. Please resolve the merge conflicts. |
MacArgs
is confusing. It has three variants. It's used in twodifferent cases.
This commit separates these two cases.
AttrArgs
keeps much of thefunctionality of the old
MacArgs
. The newMacArgs
is a greatlystreamlined version of the old
MacArgs
. There is a small amount ofcode duplication, but I think this change improves readability. It also
lets a few unreachable code paths (e.g.
unwrap
/expect
calls) beremoved.
The commit also renames
MacDelimeter
asAttrMacDelimeter
, becauseit's used for both
AttrArgs
andMacArgs
. For this type, it makesmore sense to share a single type between the two cases because all
variants and methods are used in both cases.
r? @matthiaskrgr