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

[Erich Keane via Phabricator via cfe-commits]

erichkeane 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.

Right, there is an Identifier type (and collection type) in Attr.td that at least a handful of attributes are using intentionally.  It is typically a case where GCC accepted an identifier, and it isn't always something that is look-up-able via normal C++ mechanisms (such as in the case of `cpu_dispatch`, where the identifiers are CPU names).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94092