aaron.ballman added inline comments.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:447
Actions.CorrectDelayedTyposInExpr(ParseAssignmentExpression()));
+
+ if (Tok.is(tok::ellipsis)) {
----------------
erichkeane wrote:
> steffenlarsen wrote:
> > erichkeane wrote:
> > > steffenlarsen wrote:
> > > > erichkeane wrote:
> > > > > steffenlarsen wrote:
> > > > > > erichkeane wrote:
> > > > > > > So I was thinking about this overnight... I wonder if this logic
> > > > > > > is inverted here? Aaron can better answer, but I wonder if we
> > > > > > > should be instead detecting when we are on the 'last' parameter
> > > > > > > and it is one of these `VariadicExprArgument` things (that accept
> > > > > > > a pack), then dropping the parser into a loop of:
> > > > > > >
> > > > > > > while (Tok.is(tok::comma)){
> > > > > > > Tok.Consume();
> > > > > > > ArgExpr = CorrectTypos(ParseAssignmentExpr(...));
> > > > > > > }
> > > > > > > // finally, consume the closing paren.
> > > > > > >
> > > > > > > WDYT?
> > > > > > The parsing of attribute arguments is already this lenient. It does
> > > > > > not actually care how many arguments the attribute says it takes
> > > > > > (unless it takes a type and then it will only currently supply
> > > > > > _one_ argument). It simply consumes as many expressions it can
> > > > > > before reaching the end parenthesis, then leaves the attributes to
> > > > > > consume them in `clang/lib/Sema/SemaDeclAttr.cpp`.
> > > > > > As a result we do not need any additional work here to handle
> > > > > > variadic arguments, but it also means that we have to introduce the
> > > > > > restrictions that we can only allow packs in the last argument and
> > > > > > allow only that argument to be variadic, as otherwise we have no
> > > > > > way of knowing when and where the packs pass between argument
> > > > > > boundaries.
> > > > > My point is, I don't think THIS function should be handling the
> > > > > ellipsis, the expression parsing functions we send the parsing of
> > > > > each component off to should do that.
> > > > >
> > > > > We unfortunately cannot move the entirety of this parsing section to
> > > > > do that, since 'identifier'/'type', and 'Function' argument types end
> > > > > up needing special handling, but it would be nice if our 'expr' types
> > > > > would all be able to do this, and I suggested earlier that we could
> > > > > start with the `VariadicExprArgument` to make this an easier patch
> > > > > that didn't have to deal with the issues that come with that.
> > > > >
> > > > > That said, it would be nice if we could get CLOSE to that.
> > > > Sorry. I am still not sure I am following. Do you mean the consumption
> > > > of pack expressions should be moved into the expression parsing so
> > > > other places (not just attributes) would accept it? If so, I am not
> > > > opposed to the idea though I have no idea which expressions these would
> > > > be nor the full implications of such a change. For these we at least
> > > > have the isolation of having to explicitly support the packs in the
> > > > given attributes.
> > > I'm saying the other expression parsers should ALREADY properly handle
> > > packs, and we should just use those.
> > I think I see what you mean. There is the `Parser::ParseExpressionList`
> > which may fit the bill, though it seems to also accept initializer lists.
> > But since it is an expression the attributes can decide if it is a valid
> > expression for them, so maybe that is not a problem?
> Hmm... I don't have a good idea of that, Aaron has better visibility into
> that sort of thing.
>
> Part of the advantage to doing it the way I suggest is that it ends up being
> a single call (that is, the parser should parse the commas!), so we should
> only have to consume the closing paren.
I think we want something more like `Parser::ParseSimpleExpressionList()`
but... that doesn't handle packs, so those appear to be handled special:
https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseExpr.cpp#L3059.
But one of these two functions seems to be along the correct lines of what we
want (with modification).
FWIW, I like the sound of Erich's approach better than what's in the current
patch. I'd rather not try to specially handle the ellipsis so much as
recognizing when we expect an arbitrary list of variadic expression arguments
as the last "parameter" for an attribute.
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
