aaron.ballman added inline comments.
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1234
+void Parser::ParseLambdaLexedAttribute(LateParsedAttribute &LA,
+ ParsedAttributes &Attrs, Declarator &D)
{
----------------
I think this should likely be named `ParseLambdaLexedGNUAttributeArgs()`
instead, because this is pretty specific to GNU-style attribute argument lists
and shouldn't be used for other attribute syntaxes, correct?
================
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();
----------------
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."
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1258
+
+ if (Tok.is(tok::eof) && Tok.getEofData() == AttrEnd.getEofData())
+ ConsumeAnyToken();
----------------
Then I think we can assert that the token is EOF here and consume it always.
================
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,
----------------
================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1397
+ for (LateParsedAttribute *Attr : LateParsedAttrs) {
+ ParseLambdaLexedAttribute(*Attr, Attributes, D);
+ delete Attr;
----------------
I think you should add an assert before this call to ensure that all attributes
are GNU ones.
================
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,
----------------
Do we have test coverage for this change? (It seems orthogonal to your patch,
so this might be worth splitting out.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D119136/new/
https://reviews.llvm.org/D119136
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
