[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

[Aaron Ballman via Phabricator via cfe-commits]

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