cor3ntin marked an inline comment as done.
cor3ntin added inline comments.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1253-1256
+ // Due to a parsing error, we either went over the cached tokens or
+ // there are still cached tokens left, so we skip the leftover tokens.
+ while (Tok.isNot(tok::eof))
+ ConsumeAnyToken();
----------------
aaron.ballman wrote:
> This comment reads a bit oddly to me. I think it may be better as "After
> parsing attribute arguments, we've either reached the EOF token (signaling
> that parsing was successful) or we have tokens we need to consume until we
> reach the EOF."
Agreed
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1258
+
+ if (Tok.is(tok::eof) && Tok.getEofData() == AttrEnd.getEofData())
+ ConsumeAnyToken();
----------------
aaron.ballman wrote:
> Then I think we can assert that the token is EOF here and consume it always.
Agreed, that seems better
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1369
+ // we delay the parsing of gnu attributes - by reusing the mechanism used
+ // for C++ late method parsing.
+ MaybeParseAttributes(PAKM_GNU | PAKM_Declspec, Attributes,
----------------
aaron.ballman wrote:
>
It's more like there is no `__declspec` that can refer to expression so this
was never implemented
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1397
+ for (LateParsedAttribute *Attr : LateParsedAttrs) {
+ ParseLambdaLexedAttribute(*Attr, Attributes, D);
+ delete Attr;
----------------
aaron.ballman wrote:
> I think you should add an assert before this call to ensure that all
> attributes are GNU ones.
I'm not sure that gains much?
In the future if late parsing is implemented for `__declspec` this would just
work , unless we add an assert. I'd be fine adding it though.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1486
+ if (Tok.isOneOf(tok::kw_mutable, tok::arrow, tok::kw___attribute,
+ tok::kw___declspec, tok::kw_constexpr, tok::kw_consteval,
+ tok::kw___private, tok::kw___global, tok::kw___local,
----------------
aaron.ballman wrote:
> Do we have test coverage for this change? (It seems orthogonal to your patch,
> so this might be worth splitting out.)
I'm happy doing that, or to add a test somewhere.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119136/new/
https://reviews.llvm.org/D119136
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
