[PATCH] D51564: Distinguish `__block` variables that are captured by escaping blocks from those that aren't

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: include/clang/AST/Decl.h:977
+
+    unsigned EscapingByref : 1;
   };
----------------
This doesn't actually seem to be initialized anywhere.


================
Comment at: include/clang/AST/Decl.h:1422
+
+  bool isNonEscapingByref() const;
+
----------------
Documentation comments?  I'm not opposed to using `Byref` as a shorthand here, but please remember that a lot of Clang maintainers aren't familiar with the blocks extension, and those that are aren't necessarily familiar with the non-escaping optimization.


================
Comment at: include/clang/AST/Decl.h:3888
+    bool isNonEscapingByref() const {
+      return VariableAndFlags.getPointer()->isNonEscapingByref();
+    }
----------------
Please call `getVariable()` here for clarity.


================
Comment at: include/clang/Sema/ScopeInfo.h:438
+    EscapingBlocks.erase(BD);
+  }
+
----------------
Please describe the expected operation in comments here: that blocks are added by default and then removed when the expression is provably used in a non-escaping way.


================
Comment at: lib/CodeGen/CGBlocks.cpp:497
+  return VD->isNonEscapingByref() ?
+         CGF.getContext().getLValueReferenceType(VD->getType()) : VD->getType();
 }
----------------
Do you need to worry about the variable already having a reference type?


================
Comment at: lib/CodeGen/CGBlocks.cpp:1310
 
-  if (capture.fieldType()->isReferenceType())
+  if (capture.fieldType()->isReferenceType() || variable->isNonEscapingByref())
     addr = EmitLoadOfReference(MakeAddrLValue(addr, capture.fieldType()));
----------------
Isn't the field type already a reference type in this case?

You should leave a comment that you're relying on that, of course.


================
Comment at: lib/Sema/Sema.cpp:1410
+        VD->setEscapingByref();
+    }
+
----------------
It's okay to walk over an unordered set here because you're just changing memory.  However, you don't actually need it to be a set; you can just remember all the blocks in the function and then ignore them if they're marked as non-escaping.


================
Comment at: lib/Sema/Sema.cpp:1412
+
+  for (VarDecl *VD : FSI.ByrefBlockVars)
+    // __block variables might require us to capture a copy-initializer.
----------------
In contrast, it's not okay to walk over an unordered set here because the loop body can emit diagnostics and/or instantiate code.

Fortunately, you don't actually need `ByrefBlockVars` to be a set at all; you can just use an `llvm::TinyPtrVector'.


================
Comment at: lib/Sema/Sema.cpp:1414
+    // __block variables might require us to capture a copy-initializer.
+    if (VD->isEscapingByref()) {
+      QualType T = VD->getType();
----------------
Please use an early `continue` so you can de-indent the rest of the loop body.


================
Comment at: lib/Sema/Sema.cpp:1420
+      // constructing this copy.
+      if (T->isStructureOrClassType()) {
+        EnterExpressionEvaluationContext scope(
----------------
Should the contents of this block be in a helper function?


================
Comment at: lib/Sema/Sema.cpp:1451
+  // Call this function before popping the current function scope.
+  markEscapingByrefs(*FunctionScopes.back(), *this);
+
----------------
Why before?


Repository:
  rC Clang

https://reviews.llvm.org/D51564