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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
