ille added inline comments.
================
Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+ return hasAttr<BlocksAttr>() && NonParmVarDeclBits.CapturedByOwnInit;
+}
----------------
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.
================
Comment at: clang/lib/AST/Decl.cpp:2498
+ return isCapturedByOwnInit() &&
+ isa<RecordType>(getType().getAtomicUnqualifiedType());
+}
----------------
rjmccall wrote:
> You should use `->is<RecordType>()` instead of `isa`, since the latter
> doesn't handle e.g. typedefs.
>
> The reference to TEK_Aggregate is inappropriate here; it's okay to just talk
> about "aggregates" under the idea that anything else could validly just be
> zero-initialized prior to the capture.
You mean `isRecordType()`? Good catch, I'll add a test for that.
================
Comment at: clang/lib/Sema/Sema.cpp:1949
+ return nullptr;
+}
+
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90434/new/
https://reviews.llvm.org/D90434
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
