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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/Analysis/AnalysisDeclContext.cpp:173
+    ValueDecl *VD = LC.getCapturedVar();
+    if (isSelfDecl(dyn_cast<VarDecl>(VD)))
       return dyn_cast<ImplicitParamDecl>(VD);
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > This looks dangerous -- `isSelfDecl()` uses the pointer variable in ways that would be Bad News for null pointers.
> Yep, `isSelfDecl` seems to do:
> 
> `return isa<ImplicitParamDecl>(VD) && VD->getName() == "self";`
> 
> `isa` isn't nullptr safe, we have `isa_and_nonnull` for that (if we want to update `isSelfDecl`).
Using `isa_and_nonnull` looks like the best solution here


================
Comment at: clang/lib/Sema/SemaDecl.cpp:14594-14596
+      ValueDecl *VD = C.getCapturedVar();
+      if (VarDecl *Var = dyn_cast<VarDecl>(VD)) {
+        if (Var->isInitCapture())
----------------
erichkeane wrote:
> 
I'm not sure I understood your suggestion.
The reason there is a local `ValueDecl` in addition of the `VarDecl` is because we use it Line 14660 just below.
But that was reworked when lifting isInitCapture in ValueDecl anyway.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16393
 
-    VarDecl *Var = Cap.getVariable();
+    VarDecl *Var = cast<VarDecl>(Cap.getVariable());
     Expr *CopyExpr = nullptr;
----------------
aaron.ballman wrote:
> Is `cast<>` safe here?
Yes, I added a comment to make that clear


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18324
+
+  assert((isa<VarDecl>(Var) || isa<BindingDecl>(Var)) &&
+         "Only variables and structured bindings can be captured");
----------------
aaron.ballman wrote:
> 
Fancy. The parentheses are still needed though, because C macros are amazing like that :)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18531
+  } else if ((BD = dyn_cast<BindingDecl>(Var))) {
+    ME = dyn_cast_or_null<MemberExpr>(BD->getBinding());
+  }
----------------
aaron.ballman wrote:
> Can this return nullptr?
I think so 
```
/// Get the expression to which this declaration is bound. This may be null
/// in two different cases: while parsing the initializer for the
/// decomposition declaration, and when the initializer is type-dependent.
Expr *getBinding() const { return Binding; }
```

The initializer could be type dependant here, and so it could be null


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


================
Comment at: clang/lib/Sema/SemaLambda.cpp:1208-1209
+    if (isa<BindingDecl>(Var)) {
+      Underlying = dyn_cast_or_null<VarDecl>(
+          cast<BindingDecl>(Var)->getDecomposedDecl());
+    } else
----------------
aaron.ballman wrote:
> Does `getDecomposedDecl()` ever actually return nullptr?
I don't think that's necessary indeed, nice catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122768