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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
