erichkeane added inline comments.
================ Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:724 + "attribute %0 does not support pack expansion in the last argument">; +def err_attribute_parm_pack_last_argument_only : Error< + "pack expansion in attributes is restricted to only the last argument">; ---------------- I don't really see why this is required? I would think the 722 would be all you would need. ================ Comment at: clang/lib/Parse/ParseDecl.cpp:450 + // Pack expansion is only allowed in the last attribute argument. + if (ArgExprs.size() + 1 < attributeNumberOfArguments(*AttrName)) { + Diag(Tok.getLocation(), ---------------- I don't think this should be diagnosed here, and I don't think it is 'right'. I think the ClangAttrEmitter should ensure that the VariadicExprArgument needs to be the 'last' thing, but I think that REALLY means "supports a pack anywhere inside of it". See my test examples below, I don't think this parsing is sufficient for that. ================ Comment at: clang/test/Parser/cxx0x-attributes.cpp:268 + void faz [[clang::annotate("B", (Is + ...))]] (); // expected-warning {{pack fold expression is a C++17 extension}} + void foz [[clang::annotate("C", Is...)]] (); } ---------------- what about: void foz [[clang::annotate("D", Is)]] (); I would expect that to error. Another test I'd like to see: void foz[[clang::annotate("E", 1, 2, 3, Is...)]] Also, I don't see why if THAT works, that: void foz[[clang::annotate("E", 1, Is..., 2, 3)]] shouldn't be allowed as well. ================ Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:1166 class VariadicExprArgument : public VariadicArgument { + bool AllowPack = false; ---------------- The rule of 'only the last argument is allowed to support a pack' should be in the attribute emitter. 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