aaron.ballman requested changes to this revision. aaron.ballman added inline comments. This revision now requires changes to proceed.
================ 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; ---------------- 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. 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
