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