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