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

[Steffen Larsen via Phabricator via cfe-commits]

steffenlarsen added a comment.

Thank you, @aaron.ballman ! I will update the revision in a moment with some of the suggested changes.



================
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);
----------------
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?


================
Comment at: clang/lib/Parse/ParseExpr.cpp:3360
+                                 llvm::function_ref<void()> ExpressionStarts,
+                                 bool FailImmediatelyOnInvalidExpr,
+                                 bool EarlyTypoCorrection) {
----------------
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.


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


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

https://reviews.llvm.org/D114439