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

[Corentin Jabot via Phabricator via cfe-commits]

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