rjmccall added inline comments.
================
Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+ return hasAttr<BlocksAttr>() && NonParmVarDeclBits.CapturedByOwnInit;
+}
----------------
ille wrote:
> rjmccall wrote:
> > You should check `isEscapingByref()` here rather than just `hasAttr`; if
> > none of the capturing blocks are escaping, we don't need to worry about
> > this. Or are you handling this implicitly in your pass during
> > escape-marking? That seems subtle.
> On second thought, the `hasAttr<BlocksAttr>()` shouldn't be there; it's
> unnecessary. (I based it on `isEscapingByref`, where it's also unnecessary -
> but it's needed in `isNonEscapingByref`, so I suppose `isEscapingByref` has
> it for symmetry.). I should replace it with just `!isa<ParmVarDecl>(this)`
> to ensure that `NonParmVarDeclBits` is valid. And the name
> `isCapturedByOwnInit` is also a misnomer; I should change it to something
> like `isByRefCapturedByOwnInit`.
>
> I want the substantive conditions for `isCapturedByOwnInit` to be handled
> during escape marking, for two reasons. One, that's where the warning is
> emitted, and I want to ensure it doesn't ever disagree with codegen about
> whether init-on-heap needs to be applied. Two, it can't mean "is captured by
> own initializer regardless of __block", because that would imply
> unnecessarily running the capture check on all variables.
>
Okay. In that case, this can just go in the header, and it's worth documenting
in comments (on either the accessor or the underlying bit) that it's only set
on `__block` variables.
================
Comment at: clang/lib/Sema/Sema.cpp:1949
+ return nullptr;
+}
+
----------------
ille wrote:
> rjmccall wrote:
> > There is an EvaluatedExprVisitor class which might make this easier, and
> > which will definitely eliminate some false positives.
> Hmm… In [an earlier revision by jfb to this same
> check](https://reviews.llvm.org/D47096?id=147614), you were concerned about
> using RecursiveASTVisitor because 'RecursiveASTVisitor instantiations are
> huge'. EvaluatedExprVisitor inherits from a different class from
> RecursiveASTVisitor, but they look pretty similar, so doesn't the same
> concern about template bloat apply? Perhaps I'm misunderstanding that
> earlier comment.
`StmtVisitor` instantiations don't come with the same code-size impact as
`RecursiveASTVisitor`, mostly because they primarily depend on the same
recursive `children()` walk rather than the explicit repeated logic and careful
tracking of the latter, but also because they don't make an effort to recurse
into various syntactic but unevaluated expressions, and because the main
"heavyweight" thing in their instantiation (the switch in `Visit`) is pretty
easily optimized by the compiler if you actually end up only caring about
particular cases. But also, `EvaluatedExprVisitor` is the right tool for this
job because you only care about evaluated references to the variable.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90434/new/
https://reviews.llvm.org/D90434
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
