[PATCH] D114439: [Annotation] Allow parameter pack expansions in annotate attribute

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:468
+
+      // General case. Parse all available expressions.
+      CommaLocsTy CommaLocs;
----------------
I think it'd be good to move this comment up closer to the `else` on line 461 so that it's clear what the else clause covers (and more clear that this is the common case).


================
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);
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > I'm a bit surprised this logic moved -- are there no times when we'd need it within the new `else` clause?
> This is done because `ParseExpressionList` wouldn't know what to do with identifiers, so it keeps the old logic. This means that attributes that consist of a variadic identifier argument cannot parse packs and initializer lists. It shouldn't be a problem for now, but it does kill some generality.
> 
> An alternative is to split the individual expression parsing from `ParseExpressionList` and use that in here in a similar way to how it parsed before. Would that be preferred?
> This is done because ParseExpressionList wouldn't know what to do with identifiers, so it keeps the old logic.

Ah, I think I confused myself into thinking `{a, b, c}` would be using a DeclRefExpr for each of the identifiers, but this code path is for identifiers that are *not* expressions. Got it.

> An alternative is to split the individual expression parsing from ParseExpressionList and use that in here in a similar way to how it parsed before. Would that be preferred?

I think this is okay for now.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+                                 llvm::function_ref<void()> ExpressionStarts,
+                                 bool FailImmediatelyOnInvalidExpr,
+                                 bool EarlyTypoCorrection) {
----------------
steffenlarsen wrote:
> aaron.ballman wrote:
> > 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.
> > "Fail immediately" doesn't quite convey the behavior to me because we fail immediately either way
> 
> Am I misunderstanding `FailUntil` or is the intention of `SkipUntil(tok::comma, tok::r_paren, StopBeforeMatch)` to either stop at the next "," or ")"? If I am not misunderstanding, I believe it will continue parsing expressions after comma if `SkipUntil` stopped before it. As such I don't think `FailImmediatelyOnInvalidExpr` is inherently wrong, but I agree that skipping the skipping isn't very well conveyed by the name as is.
You're correct about the behavior (I had missed that we stopped on a comma as well as closing paren, I was thinking we were stopping on the closing curly brace). So the parameter name isn't inherently wrong, just seems a bit unclear to me. I don't insist on a change, but if someone has a better name for the parameter, I won't be sad either.


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3374-3375
 
+    if (EarlyTypoCorrection)
+      Expr = Actions.CorrectDelayedTyposInExpr(Expr);
+
----------------
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?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114439/new/

https://reviews.llvm.org/D114439