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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
