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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:447
+          }
+          ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+        }
----------------
steffenlarsen wrote:
> erichkeane wrote:
> > Do you have a test for something that isn't a pack followed by an ellipsis?  What is the behavior there?   I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?
> > 
> > Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?).  I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm not particularly sure what is going to happen here?
> > Do you have a test for something that isn't a pack followed by an ellipsis? What is the behavior there? I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?
> 
> Good question! I would expect it to fail in kind during template instantiation, or earlier if it is not an expression, but I will test it out once we converge on a new solution.
> 
> > Also, why is this 'if' not up on line 429 (that is, outside of this 'else'?). I believe ParseAssignmentExpression is going to muck-up the tokens, so I'm not particularly sure what is going to happen here?
> 
> I originally tried having the `ActOnPackExpansion` earlier, but it did indeed cause trouble. However, if my understanding of the previous two branches is correct, they are:
> 
> 
>   # Type argument. This could be consuming the ellipsis as well, though I suspect it needs another path given that it does not seem to produce any expressions. Note however that it currently seems to stop argument parsing after reading the first type (confirmed by the FIXME) so parameter pack logic for this branch would probably be more suitable once it actually allows attributes to take multiple types.
>   # Identifier argument. Correct me if I am wrong, but I don't think this can be populated by a parameter pack.
> 
> I could add the diagnostic to these if ellipsis is found, but if my assumptions are correct it doesn't make much sense to expand these branches beyond that.
> I'm also concerned that there doesn't seem to be anything here that handles complex-pack expansions (like folds), can you test that as well?

Sorry, I forgot to address this. I have not tested it, but I would expect complex-pack expansions like folds to be at expression-level. I will make sure to test this as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114439