[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:386
   ArgsVector ArgExprs;
   if (Tok.is(tok::identifier)) {
     // If this attribute wants an 'identifier' argument, make it so.
----------------
steffenlarsen wrote:
> erichkeane wrote:
> > So does this mean that our pack-parsing code is allowing identifiers?  I really expected this patch to enforce via the clang-attr-emitter that ONLY the 'expr' types were allowed.  So you'd have:
> > 
> > if (attributeAcceptsExprPack(AttrName)) {
> > ... <where this ONLY accepts an expression pack>
> > } else if (Tok.is(tok::identifier)) {
> > ...
> > }...
> This will only consume the identifier if the argument is an identifier argument (variadic or non-variadic). As you say, clang-attr-emitter will make sure that no attribute with `AcceptsExprPack` has any identifier arguments, so if there is an identifier here it will not be able to consume it and then it will continue on to try and parse it as an expression and fail.
> 
> That said, the second part of the following parsing branches should probably also check `attributeHasIdentifierArg(*AttrName)`, so I will add that shortly.
> That said, the second part of the following parsing branches should probably also check `attributeHasIdentifierArg(*AttrName)`, so I will add that shortly.

Correction: Parsing any identifiers after this assumes there is a variadic identifier argument. Effectively it assumes that all non-variadic identifiers in an attribute are at the start of an attribute arguments, in which case they'll be parsed here. This is a little lackluster as it could simply allow identifiers in other places if there are variadic identifier arguments in the attribute, but I don't think fixing it should be in the scope of this patch.


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

https://reviews.llvm.org/D114439