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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
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;
+  }
----------------
ChuanqiXu wrote:
> It looks like this one is not used, is it?
Thanks., you are right, i wrote that before realizing i did not need it.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:842
+
+  bool BeforeLambdaQualifiersScope = false;
+
----------------
ChuanqiXu wrote:
> 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...
I struggle to find a better name, but I added a comment. Does that help? 


================
Comment at: clang/include/clang/Sema/Sema.h:6848
+
+  // 1: Create the class
+  void ActOnLambdaIntroducer(LambdaIntroducer &Intro, Scope *CurContext);
----------------
ChuanqiXu wrote:
> The comment looks like a draft.
Thanks for noticing that, I added a longer comment


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1439
+    DeclEndLoc = RParenLoc = T.getCloseLocation();
+    HasParameters = true;
+  }
----------------
ChuanqiXu wrote:
> It looks a little bit. It looks like if the code is `()` and `HasParameters` would still be true. 
Which is what we want to track. But it was not clear, I renamed the variable to `HasParentheses`


================
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:
> ChuanqiXu wrote:
> > This looks like:
> This isn't addressed. I mean the flags would always be true if the last clause get evaluated.
I'm not sure I understand your point. We are checking that it's a function scope that is not a lambda scope.
They are probably other ways to write that but they aren't clearer


================
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,
----------------
ChuanqiXu wrote:
> The comment says `Start` while the function name starts with `finish`. It looks odd...
The comment appertain to the `startLambdaDefinition` below, i fixed it. 
I also renamed `finishLambdaMethodType` to buildTypeForLambdaCallOperator` which is clearer`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119136