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

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:843
+  /// and the capture should not be visible before.
+  llvm::DenseMap<unsigned, DelayedCapture> DelayedCaptures;
+
----------------
I hope the comment explains the meaning of unsigned here. If it means index, I think `SmallVector` would make code simpler.


================
Comment at: clang/include/clang/Sema/ScopeInfo.h:842
+
+  bool BeforeLambdaQualifiersScope = false;
+
----------------
cor3ntin wrote:
> 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? 
I'm not good at naming too... I am wondering a name with words like `range` could be fine.


================
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++;
----------------
cor3ntin wrote:
> 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
I mean the suggested change is equal to the original one and it is shorter. I don't think it would lost any information since they are all in the same line.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18411
+    Loc = C.second.Loc;
+    Explicitly = 1;
+    break;
----------------



================
Comment at: clang/lib/Sema/SemaLambda.cpp:892
 
-void Sema::ActOnStartOfLambdaDefinition(LambdaIntroducer &Intro,
-                                        Declarator &ParamInfo,
-                                        Scope *CurScope) {
+static LambdaScopeInfo *getCurrentLambdaScopeUnsafe(Sema &S) {
+  assert(!S.FunctionScopes.empty());
----------------
I guess it is needed to explain the meaning of `Unsafe`


================
Comment at: clang/lib/Sema/SemaLambda.cpp:894-897
+  auto *CurLSI =
+      dyn_cast<LambdaScopeInfo>(S.FunctionScopes[S.FunctionScopes.size() - 1]);
+  assert(CurLSI);
+  return CurLSI;
----------------



================
Comment at: clang/lib/Sema/SemaLambda.cpp:1076-1078
       Var = createLambdaInitCaptureVarDecl(C->Loc, C->InitCaptureType.get(),
                                            C->EllipsisLoc, C->Id, InitStyle,
+                                           C->Init.get(), Method);
----------------
I suggest to add a assertion for `Var` to check it is not null


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1127-1133
+      auto FindVar = [LSI](VarDecl *V) {
+        auto it = std::find_if(
+            LSI->DelayedCaptures.begin(), LSI->DelayedCaptures.end(),
+            [&](auto &&Pair) { return Pair.second.Var == V; });
+        return it;
+      };
+      auto it = FindVar(Var);
----------------



================
Comment at: clang/lib/Sema/SemaLambda.cpp:1182-1184
+    if (Var)
+      LSI->DelayedCaptures[I] = LambdaScopeInfo::DelayedCapture{
+          Var, C->ExplicitRange.getBegin(), C->Kind};
----------------
If `DelayedCaptures` is SmallVector, we could write:

---

I observed there a lot `continue` in the loop. So we could hoist the definition of `Var` to the start of the loop. And we could fill `DelayedCaptures` by RAII.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1193
+                                         LambdaIntroducer &Intro) {
+  unsigned I = 0;
+  SourceLocation PrevCaptureLoc;
----------------
Do you think it is better to replace `I` with `std:distance(Intro.Captures.begin(), C)`? I prefer it since  we could reduce the scope of `I` in the style.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1194
+  unsigned I = 0;
+  SourceLocation PrevCaptureLoc;
+  for (auto C = Intro.Captures.begin(), E = Intro.Captures.end(); C != E;
----------------
It looks like we could define `PrevCaptureLoc` in the loop.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1214
+
+      // C++2a [expr.prim.lambda]p8:
+      //  If a lambda-capture includes a capture-default that is =,
----------------
We prefer C++20 than C++2a now.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1311-1312
+
+  if (ParamInfo.getTrailingRequiresClause())
+    Method->setTrailingRequiresClause(ParamInfo.getTrailingRequiresClause());
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119136