[PATCH] D122768: [Clang][C++20] Support capturing structured bindings in lambdas

[Corentin Jabot via Phabricator via cfe-commits]

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