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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

CodeGen tests is still not testing what we intend I believe (sorry if I missed the response to this!), plus I would like a quick explanation as to what is going on in 1 place, otherwise I think this is about right.  Hopefully Aaron/Shafik can take a quick look through to confirm as well.



================
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()));
----------------
erichkeane wrote:
> aaron.ballman wrote:
> > I think early returns help make this a bit more clear.
> I might suggest making that:
> 
> ```
> if (const auto *VD = dyn_cast<VarDecl(C->getCapturedVar())
>    return VD->...
> return false;
> ```
> 
> But otherwise agree with the suggestion.
FWIW, this seems nicer now without the casts.  I'm OK with this form, it is exactly what there was before minus some strange formatting/parens.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18399
+      if (Diagnose)
+        diagnoseUncapturableValueReferenceOrBinding(S, Loc, Var);
+      return false;
----------------
first, why are we checking for !CPlusPlus... aren't BindingDecl's C++ only?  Or is there some ObjC thing for them?

second, can you clarify what the two diagnostic cases are doing?  it isn't completely clear to me the purpose of the first call here.


================
Comment at: clang/test/CodeGenCXX/cxx20-decomposition.cpp:26
+// CHECK: %{{.*}} = load ptr, {{.*}}
+// CHECK: %{{.*}} = load i32, {{.*}}
+// CHECK: %{{.*}} = getelementptr {{.*}}, i32 0, i32 0
----------------
cor3ntin wrote:
> erichkeane wrote:
> > Which is the important lines here?  You might want to use the `[[NAME:.whatever]]`  (then on the 'other' side: `[[NAME]]`)syntax in here to make sure that the check-lines don't find something else.
> > 
> > You also likely want to use `.+` to make sure there is actually a character in there.
> > 
> > 
> I'm trying to show i is captured by value and j isn't
Hmm... I don't think it is testing what you think you're testing, particularly with so many unnamed checks.  If you send me the IR that this produces and highlight which are the 'important' lines, I can help write this for you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122768