[PATCH] D90434: [CodeGen] Correct codegen for self-capturing __block var

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

This is great work, thank you.



================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:5349
+def note_because_captured_by_block : Note<
+  "because it is captured by a block used in its own initializer">;
+
----------------
This will read okay on the command line, but it'll be awkward in most IDEs that consume diagnostics.  You should report the problem fairly completely in the main diagnostic and then just say something like "captured in block here" in the note.


================
Comment at: clang/lib/AST/Decl.cpp:2491
+bool VarDecl::isCapturedByOwnInit() const {
+  return hasAttr<BlocksAttr>() && NonParmVarDeclBits.CapturedByOwnInit;
+}
----------------
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.


================
Comment at: clang/lib/AST/Decl.cpp:2498
+  return isCapturedByOwnInit() &&
+         isa<RecordType>(getType().getAtomicUnqualifiedType());
+}
----------------
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.


================
Comment at: clang/lib/CodeGen/CGBlocks.cpp:2754
+
+  BuildBlockRelease(addr.getPointer(), flags, false);
+}
----------------
I think there's some tooling that will be slightly happier if you release the value in `out` instead of `addr`.  I believe we do emit a different function call to enable that in unoptimized builds.

You should add comments describing what you're doing here.  The explanation from the commit description's pretty good.


================
Comment at: clang/lib/Sema/Sema.cpp:1949
+  return nullptr;
+}
+
----------------
There is an EvaluatedExprVisitor class which might make this easier, and which will definitely eliminate some false positives.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90434