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

[John McCall via Phabricator via cfe-commits]

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