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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D119136#3421531 <https://reviews.llvm.org/D119136#3421531>, @cor3ntin wrote:

> In D119136#3418585 <https://reviews.llvm.org/D119136#3418585>, @ChuanqiXu wrote:
>
>> I've just skimmed over the patch and it is a little bit hard to understand for people who don't have a strong background. Is it possible to split this one into several parts?
>
> Unfortunately, I don't thing that would be very practical. The patch is not small but what it changes is rather small, and if we tried to split it the intermediate patches would be inconsistent and untested

Yeah, but I indeed found some NFC changes... I think it might be helpful to split it as NFC parts and other parts.



================
Comment at: clang/include/clang/AST/DeclCXX.h:1816-1821
+  void setLambdaIsDependent(bool IsDependent) {
+    auto *DD = DefinitionData;
+    assert(DD && DD->IsLambda && "setting lambda property of non-lambda class");
+    auto &DL = static_cast<LambdaDefinitionData &>(*DD);
+    DL.DependencyKind = IsDependent ? LDK_AlwaysDependent : LDK_Unknown;
+  }
----------------
It looks like this one is not used, is it?


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:840
+
+  llvm::DenseMap<unsigned, DelayedCapture> DelayedCaptures;
+
----------------
I suggest a comment to describe this one.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:842
+
+  bool BeforeLambdaQualifiersScope = false;
+
----------------
I think the name `Before*` is a little bit confusing. From the context, it would be set to true in `ActOnLambdaIntroducer`and be set to false in `ActOnLambdaClosureQualifiers`. So it looks like something `IsInRangeOf...`. The name before implies that it should be true by default and be false after some point...

I am not sure if my point is clear...


================
Comment at: clang/include/clang/Sema/Sema.h:6848
+
+  // 1: Create the class
+  void ActOnLambdaIntroducer(LambdaIntroducer &Intro, Scope *CurContext);
----------------
The comment looks like a draft.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1388
             DeclEndLoc = Range.getEnd();
-        }
+        };
 
----------------
Unintended change?


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1439
+    DeclEndLoc = RParenLoc = T.getCloseLocation();
+    HasParameters = true;
+  }
----------------
It looks a little bit. It looks like if the code is `()` and `HasParameters` would still be true. 


================
Comment at: clang/lib/Sema/Scope.cpp:72
+  // Lambdas have an extra prototype scope that doesn't add any depth
+  if (flags & FunctionPrototypeScope && !(flags & LambdaScope))
+    PrototypeDepth++;
----------------
ChuanqiXu wrote:
> This looks like:
This isn't addressed. I mean the flags would always be true if the last clause get evaluated.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18247
+  SourceLocation Loc = LSI->IntroducerRange.getBegin();
+  unsigned Explicitly = 0;
+  for (auto &&C : LSI->DelayedCaptures) {
----------------
Could we declare it as a boolean.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:358-360
+/// Start the definition of a lambda expression.
+/// In this overload, we do not know the type yet
+static QualType finishLambdaMethodType(Sema &S, clang::CXXRecordDecl *Class,
----------------
The comment says `Start` while the function name starts with `finish`. It looks odd...


================
Comment at: clang/lib/Sema/SemaLambda.cpp:511-513
+      if (S.RequireCompleteType(CallOperator->getBeginLoc(), LSI->ReturnType,
+                                diag::err_lambda_incomplete_result)) {
+        // Do nothing.
----------------
How about:


================
Comment at: clang/lib/Sema/SemaLambda.cpp:893
 
-void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
-                                        Declarator &ParamInfo,
-                                        Scope *CurScope) {
+LambdaScopeInfo *getCurrentLambdaScopeUnsafe(Sema &S) {
+  assert(!S.FunctionScopes.empty());
----------------
Add a `static` qualifier.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119136/new/

https://reviews.llvm.org/D119136