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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Stmt.h:62-63
 class Token;
 class VarDecl;
+class ValueDecl;
 
----------------
We usually keep these alphabetical.


================
Comment at: clang/lib/AST/ExprCXX.cpp:1214-1216
+  return (C->capturesVariable() && isa<VarDecl>(C->getCapturedVar()) &&
+          cast<VarDecl>(C->getCapturedVar())->isInitCapture() &&
           (getCallOperator() == C->getCapturedVar()->getDeclContext()));
----------------
I think early returns help make this a bit more clear.


================
Comment at: clang/lib/AST/StmtPrinter.cpp:2167
     if (Node->isInitCapture(C)) {
-      VarDecl *D = C->getCapturedVar();
+      VarDecl *D = cast<VarDecl>(C->getCapturedVar());
 
----------------



================
Comment at: clang/lib/Analysis/AnalysisDeclContext.cpp:173
+    ValueDecl *VD = LC.getCapturedVar();
+    if (isSelfDecl(dyn_cast<VarDecl>(VD)))
       return dyn_cast<ImplicitParamDecl>(VD);
----------------
This looks dangerous -- `isSelfDecl()` uses the pointer variable in ways that would be Bad News for null pointers.


================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:9060
         continue;
-      const VarDecl *VD = LC.getCapturedVar();
+      const VarDecl *VD = cast<VarDecl>(LC.getCapturedVar());
       if (LC.getCaptureKind() != LCK_ByRef && !VD->getType()->isPointerType())
----------------
cor3ntin wrote:
> cor3ntin wrote:
> > I'm not sure we currently prohibit capturing a structured binding in openmp, i should fix that
> I disabled the feature in OpenMP mode, as this needs more investigation
Ping @jdoerfert -- do you happen to know what's up here?


================
Comment at: clang/lib/Sema/ScopeInfo.cpp:223-224
   // init-capture.
-  return !isNested() && isVariableCapture() && getVariable()->isInitCapture();
+  return !isNested() && isVariableCapture() && isa<VarDecl>(getVariable()) &&
+         cast<VarDecl>(getVariable())->isInitCapture();
 }
----------------
This also has a bad code smell for `isa<>` followed by `cast<>`, but I'm not certain if the rewrite is actually better or not.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16393
 
-    VarDecl *Var = Cap.getVariable();
+    VarDecl *Var = cast<VarDecl>(Cap.getVariable());
     Expr *CopyExpr = nullptr;
----------------
Is `cast<>` safe here?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18324
+
+  assert((isa<VarDecl>(Var) || isa<BindingDecl>(Var)) &&
+         "Only variables and structured bindings can be captured");
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:18527
+  BindingDecl *BD = nullptr;
+  if (VarDecl *V = dyn_cast_or_null<VarDecl>(Var)) {
+    if (V->getInit())
----------------
I'm assuming, given that the function didn't used to accept nullptr for `Var` and the `else if` is using `dyn_cast<>`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18531
+  } else if ((BD = dyn_cast<BindingDecl>(Var))) {
+    ME = dyn_cast_or_null<MemberExpr>(BD->getBinding());
+  }
----------------
Can this return nullptr?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18534-18535
+
+  // Capturing a bitfield by reference is not allowed
+  // except in OpenMP mode
+  if (ByRef && ME &&
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:18539
+       !S.isOpenMPCapturedDecl(Var))) {
+    FieldDecl *FD = dyn_cast_or_null<FieldDecl>(ME->getMemberDecl());
+    if (FD &&
----------------
Can this return nullptr?


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18551
+  }
+  // FIXME: We should support capturing structured bindings in OpenMP
+  if (!Invalid && BD && S.LangOpts.OpenMP) {
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:18749
   DeclContext *VarDC = Var->getDeclContext();
-  if (Var->isInitCapture())
-    VarDC = VarDC->getParent();
+  VarDecl *VD = dyn_cast<VarDecl>(Var);
+  if (VD) {
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:18750-18756
+  if (VD) {
+    if (VD && VD->isInitCapture())
+      VarDC = VarDC->getParent();
+  } else {
+    VD = dyn_cast<DecompositionDecl>(
+        cast<BindingDecl>(Var)->getDecomposedDecl());
+  }
----------------



================
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)
----------------
shafik wrote:
> cor3ntin wrote:
> > shafik wrote:
> > > 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.
> > I considered having a function In ValueDecl, what do you think?
> I thought about that but when I looked at `ValueDecl` it seemed pretty lightweight.
> 
> @aaron.ballman wdyt is `ValueDecl` the right place or is this code repetition ok? 
I think it makes sense to sink this into `ValueDecl` given how often it's come up. We really try to discourage people from doing `isa<>` followed by a `cast<>` as that's a bad code smell for `dyn_cast<>`.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1169
 
-      Var = R.getAsSingle<VarDecl>();
+      if (auto BD = R.getAsSingle<BindingDecl>())
+        Var = BD;
----------------



================
Comment at: clang/lib/Sema/SemaLambda.cpp:1208-1209
+    if (isa<BindingDecl>(Var)) {
+      Underlying = dyn_cast_or_null<VarDecl>(
+          cast<BindingDecl>(Var)->getDecomposedDecl());
+    } else
----------------
Does `getDecomposedDecl()` ever actually return nullptr?


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1717
   if (Capture.isVariableCapture()) {
-    auto *Var = Capture.getVariable();
-    if (Var->isInitCapture())
-      TSI = Capture.getVariable()->getTypeSourceInfo();
+    VarDecl *Var = llvm::dyn_cast_or_null<VarDecl>(Capture.getVariable());
+    if (Var && Var->isInitCapture())
----------------



================
Comment at: clang/lib/Sema/TreeTransform.h:12989
     TransformedInitCapture &Result = InitCaptures[C - E->capture_begin()];
-    VarDecl *OldVD = C->getCapturedVar();
+    VarDecl *OldVD = cast<VarDecl>(C->getCapturedVar());
 
----------------



================
Comment at: clang/lib/Sema/TreeTransform.h:13174
 
-      VarDecl *OldVD = C->getCapturedVar();
+      VarDecl *OldVD = cast<VarDecl>(C->getCapturedVar());
       llvm::SmallVector<Decl*, 4> NewVDs;
----------------
I'll stop commenting on these -- you should take a pass over your changes and use `const auto *`` or `auto *` anywhere the type is spelled on the RHS via `cast<>`, `dyn_cast<>`, or similar.


================
Comment at: clang/www/cxx_status.html:1143
       <td><a href="https://wg21.link/p1091r3">P1091R3</a></td>
-      <td rowspan="2" class="partial" align="center">Partial</td>
+      <td rowspan="2" class="unreleased" align="center">Clang 15</td>
     </tr>
----------------



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122768/new/

https://reviews.llvm.org/D122768