shafik added a comment. Herald added a reviewer: NoQ. Thank you for this work. This looks mostly good to me but I am not confident on all the code. I feel like there are some assumptions that maybe could use comments e.g. places where we know it can only be a `VarDecl` e.g. b/c it is an init capture.
================ Comment at: clang/lib/CodeGen/CGExpr.cpp:2933 + auto *FD = LambdaCaptureFields.lookup(BD); + assert(FD); + return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue); ---------------- Why the `assert`? In the previous use of `LambdaCaptureFields.lookup(...)` we are not doing that. ================ Comment at: clang/lib/Sema/SemaExpr.cpp:18269 return getLambdaAwareParentOfDeclContext(DC); - else if (Var->hasLocalStorage()) { - if (Diagnose) - diagnoseUncapturableValueReference(S, Loc, Var); + else if (VarDecl *VD = dyn_cast<VarDecl>(Var)) { + if (VD->hasLocalStorage() && Diagnose) ---------------- What are we checking here? Why does it only apply to `VarDecl`s? ================ Comment at: clang/lib/Sema/SemaExpr.cpp:18341 + if (isa<BindingDecl>(Var)) { + if (!IsLambda || !S.getLangOpts().CPlusPlus) { ---------------- This block does not follow the pattern of the blocks above, where they do `if (Diagnose)` and always return false. Your `else if` branch diagnoses but does not `return false`. ================ Comment at: clang/lib/Sema/SemaInit.cpp:7851 + bool InitCapture = + isa<VarDecl>(VD) && cast<VarDecl>(VD)->isInitCapture(); Diag(Elem.Capture->getLocation(), diag::note_lambda_capture_initializer) ---------------- I see we are doing this kind of check to see if we have a `VarDecl` and then check if it is an init capture and I wish there was a way not to repeat this but I don't see it. ================ Comment at: clang/lib/Sema/SemaLambda.cpp:1213 + + if (!Underlying->hasLocalStorage()) { Diag(C->Loc, diag::err_capture_non_automatic_variable) << C->Id; ---------------- We also do a `hasLocalStorage()` check in `getParentOfCapturingContextOrNull(...)` but only look for the `VarDecl` case there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122768/new/ https://reviews.llvm.org/D122768 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits