steffenlarsen added inline comments.
================
Comment at: clang/include/clang/Basic/Attr.td:544-546
+ // Set to true for attributes that support parameter pack expansion in its
+ // arguments.
+ bit ParmExpansionArgsSupport = 0;
----------------
aaron.ballman wrote:
> From a design perspective, I think this is a property of the parameters of an
> attribute rather than of an attribute itself. So I'd rather not add a bit to
> the `Attr` class, but instead modify the `Argument` class. There are a few
> approaches to that which I can think of, and I'm not certain which is the
> best approach.
>
> 1) Introduce `ExpressionPackArgument` and attributes can opt into using it if
> they want to support packs.
> 2) Add this bit to `VariadicExprArgument` and attributes can opt into using
> it if they want to support packs.
> 3) Change `VariadicExprArgument` to also mean "allows packs" and all
> attributes using this automatically support packs.
>
> I think that my conservative preference is for #2, but my intuition is that
> #3 is where we probably ultimately want to get to.
>
> There are definitely some open questions with this approach that we'll have
> to figure out:
>
> * Can you mix packs with variadics in the same attribute?
> * Can you have two pack arguments in the same attribute?
> * Does the tablegen design also seem likely to work when we want to introduce
> type packs instead of value packs?
>
> I think the safest bet is to be conservative and not allow mixing packs with
> variadics, and not allowing multiple packs. We should be able to diagnose
> that situation from ClangAttrEmitter.cpp to help attribute authors out.
> However, it'd be worth adding a FIXME comment to that diagnostic logic asking
> whether we want to relax the behavior at some point. But if you feel strongly
> that we should support these situations initially, I wouldn't be opposed.
Having `ParmExpansionArgsSupport` in `Attr` would allow attributes to populate
with parameter packs across argument boundaries. It seems more permissive, but
frankly I have no attachment to it, so I am okay with tying it to `Argument`
instead.
Of the mentioned options, I agree #2 and #3 are the preferred paths. For
simplicity I prefer #2, though I will have to figure out how best to add a
check on individual arguments, which @erichkeane's suggestion to limit the
attributes to only allowing parameter packs in variadic arguments as the last
argument would help with. On the other hand, the population logic for most of
the relevant attributes are explicitly defined in
`clang/lib/Sema/SemaDeclAttr.cpp` (which will have to be changed with #3) so
being more permissive - like the current version of the patch is - gives more
freedom to the attributes.
> Does the tablegen design also seem likely to work when we want to introduce
> type packs instead of value packs?
I am not sure about how tablegen would take it, but the parser doesn't
currently parse more than a single type argument, so I don't see a need for
allowing type packs at the moment.
> I think the safest bet is to be conservative and not allow mixing packs with
> variadics, and not allowing multiple packs. We should be able to diagnose
> that situation from ClangAttrEmitter.cpp to help attribute authors out.
> However, it'd be worth adding a FIXME comment to that diagnostic logic asking
> whether we want to relax the behavior at some point. But if you feel strongly
> that we should support these situations initially, I wouldn't be opposed.
If we limit packs to `VariadicExprArgument` - like `annotate` does in the
current state of this patch - having multiple packs in the same argument is
simple to handle. Both are represented as `Expr` and the variadic argument will
simply contain two expressions until the templates are instantiated and the
packs are expanded. This is also a feature I would like to keep, though it
should be possible to work around the limitation if we conclude it can't stay.
================
Comment at: clang/lib/Parse/ParseDecl.cpp:447
+ }
+ ArgExpr = Actions.ActOnPackExpansion(ArgExpr.get(), ConsumeToken());
+ }
----------------
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.
================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:261
-template<typename...Ts> void variadic() {
+template <int... Is> void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot
be used as an attribute pack}}
----------------
erichkeane wrote:
> What is the point of this change?
It makes more sense for the new test cases to get expressions rather than types
and since `Ts` wasn't used for its contents it did not seem like an intrusive
change.
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
