aaron.ballman added a comment.
Thanks for this, I think it's shaping up well!
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:723
+def err_attribute_argument_parm_pack_not_supported : Error<
+ "attribute %0 does not support pack expansion in arguments">;
+def err_attribute_parm_pack_last_argument_only : Error<
----------------
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:725
+def err_attribute_parm_pack_last_argument_only : Error<
+ "pack expansion in %0 is restricted to argument %1 or later">;
def err_ms_declspec_type : Error<
----------------
================
Comment at: clang/lib/Parse/ParseDecl.cpp:436-438
+ // Interpret "kw_this" as an identifier if the attributed requests it.
+ if (ChangeKWThisToIdent && Tok.is(tok::kw_this))
+ Tok.setKind(tok::identifier);
----------------
I'm a bit surprised this logic moved -- are there no times when we'd need it
within the new `else` clause?
================
Comment at: clang/lib/Parse/ParseDecl.cpp:497
+ // Pack expansion is only allowed in the variadic argument at the end.
+ const size_t attrNumArgs = attributeNumberOfArguments(*AttrName);
+ if (ArgExprs.size() + I + 1 < attrNumArgs) {
----------------
Coding style nits.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+ llvm::function_ref<void()> ExpressionStarts,
+ bool FailImmediatelyOnInvalidExpr,
+ bool EarlyTypoCorrection) {
----------------
or something along those lines (note, the logic has reversed meaning). "Fail
immediately" doesn't quite convey the behavior to me because we fail
immediately either way, it's more about whether we attempt to skip tokens to
get to the end of the expression list. I'm not strongly tied to the name I
suggested either, btw.
================
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+ if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
----------------
Not that I think this is a bad change, but it seems unrelated to the patch. Can
you explain why you made the change?
================
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}}
----------------
steffenlarsen wrote:
> 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.
Non-type templates are different from type templates, so I don't really want to
lose the test coverage for that (if nothing else, it ensures the parser doesn't
explode when it encounters one). It's fine to add a `variadic_nttp()` function
or something along those lines.
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
