fhahn added a comment. In D94092#2482035 <https://reviews.llvm.org/D94092#2482035>, @erichkeane wrote:
> 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). > > EDIT: > That said, I believe @rjmccall is correct, I think based on my run through > history (and knowing a bit about it from past digging) that the `vector_size` > attribute was added before attribute parsing did nearly as much as it did (I > think it even predates Attr.td). > > I suspect the rest of the attributes (Particularly Matrix and Address Space!) > simply copy/pasted that section and edited it to work, despite not having a > reproducer that hit it. For the matrix_type attribute, I originally just copied & adjusted the ext_vector_type version. Thanks for all the comments, I'll go ahead and land this now. We should know soon enough if there's anything out in the wild that can pass an `ArgIdent`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94092/new/ https://reviews.llvm.org/D94092 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
