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