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

[Shafik Yaghmour via Phabricator via cfe-commits]

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