rjmccall added inline comments.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1638
+ switch (E.Kind) {
+ case BlockCaptureEntityKind::CXXRecord: {
+ Name += "c";
----------------
ahatanak wrote:
> 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.
Great.
================
Comment at: lib/CodeGen/CGBlocks.cpp:1647
+ unsigned Qs;
+ cast<CXXConstructExpr>(CE)->getConstructor()->isCopyConstructor(Qs);
+ if (Qs & Qualifiers::Volatile)
----------------
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.
================
Comment at: lib/CodeGen/CGBlocks.cpp:492
+ if (!RD->isExternallyVisible())
+ info.CapturesNonExternalType = true;
+
----------------
You only need to do this if you're going to mangle the type name, i.e. if it's
actually going to end up as a CXXRecord capture. It should be easy enough to
just set this flag above in the clauses where you recognize interesting C++
record captures.
Repository:
rC Clang
https://reviews.llvm.org/D50152
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
