[PATCH] D50152: [CodeGen] Merge equivalent block copy/helper functions

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGBlocks.cpp:1691
+
+    Name += "_" + llvm::to_string(E.Capture->getOffset().getQuantity());
+  }
----------------
rjmccall wrote:
> I feel like this reads a little better if you write the quantity first.  Also I think you can drop the underscore because all of your suffices are easy to compute the length of, so a block that captures 3 strong variables can just be `32s40s48s` or something like that.
> 
> Oh, I guess if you do that you need to do something about how you handle the bit-mask for `BlockObject`, because that could run into the next number.  Would it be better to just break this out into cases?  The three current cases are (1) `__block` variables, which can throw, (2) blocks, and (3) object references in non-ARC modes (which for copy/dispose purposes are just `s` again, except we use a different entrypoint because we hadn't exposed `objc_retain` and `objc_release` yet).
I broke this out into several cases based on the flag's value and whether the constructor or destructor of a C++ type can throw.


================
Comment at: test/CodeGenCXX/block-byref-cxx-objc.cpp:36
+// CHECK: call void @_Block_object_dispose(
+
+int testB() {
----------------
rjmccall wrote:
> ahatanak wrote:
> > ahatanak wrote:
> > > Should the second call to @_Block_object_dispose be an invoke if the destructor can throw? The FIXME in CodeGenFunction::BuildBlockRelease seems to imply that we should consider whether the destructor can throw.
> > I mean the first call, not the second call. If the call to A's destructor throws when the first object is being destructed, the second object should be destructed on the EH path.
> Yes, I think we should assume that the block runtime correctly propagates exceptions.  The function should not be marked `nothrow`, but call sites can be marked `nothrow` depending on what they do.
> 
> This won't generally be a code-size problem because (1) we do assume that the ObjC retain/release and weak operations can't throw and (2) C++11 makes destructors `noexcept` by default.
I defined two functions, cxxDestructorCanThrow and cxxConstructorCanThrow, that are called to find out whether the `__block` object has a destructor or constructor that can throw.  

These functions can return true even when the destructor or constructor cannot throw, but that should be conservatively correct ('invoke' is emitted in some cases even when the function doesn't throw).


Repository:
  rC Clang

https://reviews.llvm.org/D50152