[cfe-commits] [PATCH] Smarter implicit copy-construction/copy-assignment.
John McCall
rjmccall at apple.com
Thu Feb 7 15:25:21 PST 2013
On Feb 5, 2013, at 6:45 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Tue, Feb 5, 2013 at 5:31 PM, Lang Hames <lhames at gmail.com> wrote:
> Hi John, Richard,
>
> Here's the updated patch for smarter copy-assignment/copy-construction for structs with Non-POD members.
>
> Richard - I've added test cases for volatile fields, inner classes, array members, pod-like members, bitfields and volatile fields.
> I've also pulled the naming scheme in line with the coding standards. I made sure I use ' &' rather than '& ' where appropriate.
>
> John -
>
> 1) Qualifiers are now being grabbed once.
> 2) I now fall back on existing codegen for when GCMode != NonGC.
> 3) Made sure I round-up to the nearest byte when copying bitfields.
> 4) I've tried to address the style issues you raised.
> 5)
>
>
> > + // Bail out on non-POD, not-trivially-constructable members.
> > + if (!FieldType.isPODType(GetCGF().getContext()) &&
> > + (!CE || !CE->getConstructor()->isTrivial()))
> > + return false;
> >
> > If you're calling a trivial copy/move constructor, it does not matter
> > whether the type is POD. Test case: give a field a type with a trivial
> > copy constructor and non-trivial destructor, or more realistically a
> > non-trivial default constructor.
>
> Note that this is the test for bailing out. The POD check needs to be there for primitive types, for which CE == 0, otherwise we'd decide that we couldn't memcpy primitive fields.
>
> Just because the type is POD, doesn't mean the selected constructor is trivial.
The point is that Lang's code was gating on *both*, which is clearly
unnecessary in the case of class types.
> Counterexample:
>
> struct S { S() = default; template<typename T> S(T&&); S(const S&) = default; };
> struct U { U(); S s; };
> static_assert(__is_pod(S), "");
> U u = static_cast<U&&>(U()); // static_cast to disable copy elision
>
> S is POD, but U's move constructor calls S's constructor template.
>
> I think this is what you mean:
>
> CE ? CE->getConstructor()->isTrivial() : FieldType.isTriviallyCopyableType(Ctx)
Mmm. This would actually drop members of reference type, which really can be safely memcpy'ed in a copy constructor.
On the other than, it would also drop members of atomic type, which really shouldn't be memcpy'ed. So there's that.
Lang, I think using Richard's proposal and then specifically opting-in a couple of extra cases (e.g. reference types) would make a lot of sense.
John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130207/15a14ff9/attachment.html>
More information about the cfe-commits
mailing list