ahatanak added inline comments.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1638
+ switch (E.Kind) {
+ case BlockCaptureEntityKind::CXXRecord: {
+ Name += "c";
----------------
rjmccall wrote:
> I forget whether this case has already filtered out trivial
> copy-constructors, but if not, you should consider doing that here.
E.Kind is set to CXXRecord only when the copy constructor is non-trivial (or
the destructor is non-trivial if this is a destroy helper). Both
computeCopyInfoForBlockCapture and computeDestroyInfoForBlockCapture filter out
types with trivial copy constructors or destructors.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1647
+ unsigned Qs;
+ cast<CXXConstructExpr>(CE)->getConstructor()->isCopyConstructor(Qs);
+ if (Qs & Qualifiers::Volatile)
----------------
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.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1651
+ }
+ std::string Str = CaptureTy->getAsCXXRecordDecl()->getName();
+ Name += llvm::to_string(Str.size()) + Str;
----------------
rjmccall wrote:
> The name of a C++ class is not unique; you need to use the mangler.
> Ideally, you would find a way to mangle all the C++ types in the context of
> the same mangler instance so that substitutions could be reused across it.
>
> You also need to check for a type with non-external linkage and make this
> helper private if you find one. (You can still unique within the translation
> unit, for what it's worth.)
Good catch. test/CodeGenCXX/blocks.cpp checks that helper functions for blocks
that capture non-external types have internal linkage.
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
