cor3ntin marked 4 inline comments as done.
cor3ntin added inline comments.
================
Comment at: clang/lib/CodeGen/CGExpr.cpp:2933
+ auto *FD = LambdaCaptureFields.lookup(BD);
+ assert(FD);
+ return EmitCapturedFieldLValue(*this, FD, CXXABIThisValue);
----------------
shafik wrote:
> Why the `assert`? In the previous use of `LambdaCaptureFields.lookup(...)`
> we are not doing that.
It can be ( and was) removed!
================
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)
----------------
shafik wrote:
> What are we checking here? Why does it only apply to `VarDecl`s?
You are right, we should check for Both kids of value. And we *did* but the
check was split
in multiple locations, which was not great, changed that. Great point.
FYI, `diagnoseUncapturableValueReferenceOrBinding` is used both to diagnose
bindings in modes they can't be captured and references to not captured
variables.
I took the liberty to rename so the name is more explicit.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:18341
+ if (isa<BindingDecl>(Var)) {
+ if (!IsLambda || !S.getLangOpts().CPlusPlus) {
----------------
shafik wrote:
> 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`.
It still somewhat not completely consistent as we only emit a warning in the
C++ case, but the check for captured bindings is now done 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
