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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
