[PATCH] D94092: [Clang] Remove unnecessary Attr.isArgIdent checks.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D94092#2481857 <https://reviews.llvm.org/D94092#2481857>, @aaron.ballman wrote:

> In D94092#2481276 <https://reviews.llvm.org/D94092#2481276>, @rjmccall wrote:
>
>> Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's responsibility to turn that back into an expression.  At some point, the parser was made sensitive to the actual attribute being parsed, but we never bothered to simplify Sema.  At any rate, the parser does now know exactly which argument of which attribute it's parsing, so there's zero reason for it to force this complexity on Sema anymore; if we find a case that parses identifier arguments, we should fix it in the parser to parse an expression instead.
>
> I don't think it will be quite that trivial (switching all identifiers to be parsed as expressions instead). For instance, attributes that take enumeration arguments can parse those arguments as identifiers, but those identifiers will never be found by usual expression lookup because they don't really exist. That said, any attribute that currently accepts an identifier because it really wants to use that identifier in an expression in Sema should update the attribute argument clauses in Attr.td and make the corresponding changes in SemaDeclAttr.cpp/SemaStmt.cpp/SemaType.cpp as appropriate.

You're exactly right about how it's necessary to parse certain attributes.  Just to be clear, though, I'm not proposing any change here; I'm describing the change that already happened, years ago, by way of trying to identify why this particular bit of code was once necessary and thus why it's likely unnecessary now.  So I think you will find that those changes to Attr.td are in fact already in place.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94092/new/

https://reviews.llvm.org/D94092