[PATCH] Don't copy objects with trivial, deleted copy ctors

Richard Smith richard at metafoo.co.uk
Thu May 8 14:12:14 PDT 2014


> Richard, you had concerns that this loop over ctors was incorrect, and that
> I should really be making some change to CXXRecordDecl::DefinitionData.
> Can you imagine some test cases that might be problematic?

Something like:

struct A {
  A(const A&) = default;
  A(const A&, int = 0);
  void *p;
};
struct B : A {};

void foo(B); // B should be passed indirect, because its copy ctor is deleted, but it's also trivial and not-yet-declared so we might not realize this.
 
> If so, do you think it's worth pursuing a more complete fix that
> updates the flags, or is it OK to accept this as an incremental fix?

It's an extremely obscure case, so the incremental fix seems reasonable to me.

================
Comment at: lib/CodeGen/CGCXXABI.cpp:49-50
@@ +48,4 @@
+
+  // If all trivial copy and move constructors are deleted, we cannot copy the
+  // argument.
+  return !(AnyDeleted && AllDeleted);
----------------
Please add a FIXME saying that we assume that all lazily-declared trivial copy/move constructors are not deleted, and that this assumption might not actually be true in some corner cases.

================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:65-66
@@ -65,1 +64,4 @@
+      return RAA_Indirect;
+    // If the class has a destructor, the ABI requires us to pass by address.
+    if (RD->hasNonTrivialDestructor())
       return RAA_Indirect;
----------------
This should probably be part of the 'canCopy' check; the proposed fix for DR1590 also requires a trivial destructor.

================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:431-435
@@ +430,7 @@
+
+    // MSVC passes objects with a trivial, non-deleted copy ctor directly.
+    for (const CXXConstructorDecl *CD : RD->ctors()) {
+      if (CD->isCopyConstructor() && CD->isTrivial() && !CD->isDeleted())
+        return RAA_Default;
+    }
+
----------------
As an optimization, you could skip this if `!hasTrivialCopyConstructor()`.

http://reviews.llvm.org/D3660






More information about the cfe-commits mailing list