[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: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