rjmccall added inline comments.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1691
+
+ Name += "_" + llvm::to_string(E.Capture->getOffset().getQuantity());
+ }
----------------
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).
================
Comment at: lib/CodeGen/CGBlocks.cpp:1647
+ unsigned Qs;
+ cast<CXXConstructExpr>(CE)->getConstructor()->isCopyConstructor(Qs);
+ if (Qs & Qualifiers::Volatile)
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > This doesn't always have to be a copy constructor. I mean, maybe this
> > > procedure works anyway, but the constructor used to copy an object isn't
> > > always a copy constructor. Blame constructor templates.
> > I've made changes to use the name of the constructor function that is used
> > to copy the capture instead of using the volatility of the copy
> > constructor's argument.
> >
> > I'm not sure when a function that is not a copy constructor would be used
> > to copy a captured object though. The comment in function captureInBlock in
> > SemaExpr.cpp says that the the blocks spec requires a const copy
> > constructor.
> Okay. I think mangling the type of the capture is probably good enough (and
> likely to be a little smaller), because different compilations using the same
> capture type will always use the same constructor. That will include
> `volatile` if the captured variable was marked `volatile`.
>
> The notable case where copy-construction doesn't use a copy constructor is
> when you have a constructor template like `template <class T> ClassName(T
> &);`, which will be preferred over the default copy-constructor for copying
> non-`const` objects. It's silly, but it's how it works.
>
> You can mangle the type by just building an `ItaniumMangleContext`. I think
> it's better to stably use the Itanium mangling instead of using the target's
> native mangling.
Itanium type manglings are self-delimiting, so you don't actually need this
run-length encoding, but it doesn't really hurt, either. Certainly it makes it
a lot easier for tools to parse one of these function names.
================
Comment at: test/CodeGenCXX/block-byref-cxx-objc.cpp:36
+// CHECK: call void @_Block_object_dispose(
+
+int testB() {
----------------
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.
Repository:
rC Clang
https://reviews.llvm.org/D50152
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
