steffenlarsen marked 2 inline comments as done.
steffenlarsen added inline comments.
================
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) {
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > Coding style nits.
> We don't typically use top-level `const`, so that should go as well.
Completely missed that. My bad!
================
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
+ if (EarlyTypoCorrection)
+ Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
----------------
aaron.ballman wrote:
> steffenlarsen wrote:
> > aaron.ballman wrote:
> > > steffenlarsen wrote:
> > > > aaron.ballman wrote:
> > > > > Not that I think this is a bad change, but it seems unrelated to the
> > > > > patch. Can you explain why you made the change?
> > > > Admittedly I am not sure why it is needed, but in the previous version
> > > > of the parsing for attribute parameters it does the typo-correction
> > > > immediately after parsing the expression and if this isn't added a
> > > > handful of tests fail.
> > > Ah, thanks for the info! So this is basically doing the same work as
> > > what's done on line 3410 in the late failure case -- I wonder if this
> > > should actually be tied to the `FailImmediatelyOnInvalidExpr` parameter
> > > instead of having its own parameter?
> > They could be unified in the current version, but I don't think there is an
> > actual relation between them.
> Do we fail tests if this isn't conditionalized at all (we always do the early
> typo correction)? (I'd like to avoid adding the new parameter because I'm not
> certain how a caller would determine whether they do or do not want that
> behavior. If we need the parameter, then so be it, but it seems like this is
> something generally useful.)
11 tests fail if we don't make it conditional. The most telling one is probably
`SemaCXX/typo-correction-delayed.cpp` which will have more errors if we do typo
correction early, so it seems that the delay is intentional.
================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:262
-template<typename...Ts> void variadic() {
+template <typename... Ts> void variadic() {
void bar [[noreturn...]] (); // expected-error {{attribute 'noreturn' cannot
be used as an attribute pack}}
----------------
aaron.ballman wrote:
> You can back out the whitespace changes for an ever-so-slightly smaller
> patch. :-D
Absolutely. Clang-format was very insistent, but since it isn't added by this
patch I shouldn't worry. 😄
================
Comment at: clang/test/Parser/cxx0x-attributes.cpp:276-277
+ void foz [[clang::annotate("E", 1, 2, 3, Is...)]] ();
+ void fiz [[clang::annotate("F", Is..., 1, 2, 3)]] ();
+ void fir [[clang::annotate("G", 1, Is..., 2, 3)]] ();
+}
----------------
aaron.ballman wrote:
> Shouldn't these cases also give the `// expected-error {{pack expansion in
> 'annotate' may only be applied to the last argument}}` diagnostic?
These should preferably pass. They are part of the "last" argument as
`annotate` has one non-variadic argument and one variadic argument. I agree
though that the error message is a little vague. Originally the error did
specify that it had to be "argument %1 or later" to try and convey this intent,
but maybe you have an idea for a clearer error message?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114439/new/
https://reviews.llvm.org/D114439
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
