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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
