[cfe-commits] [PATCH] Smarter implicit copy-construction/copy-assignment.

[Lang Hames]

Thanks very much for all the help guys. I've attached an updated patch
incorporating your feedback. It also fixes a bug -  I had been memcpying
members with defaulted but non-trivial assignment operators.

Mind if I commit?



On Thu, Feb 7, 2013 at 3:28 PM, Lang Hames <lhames at gmail.com> wrote:

> Sure thing. I'm working on an update at the moment - I'll roll this in.
>
> Cheers,
> Lang.
>
>
> On Thu, Feb 7, 2013 at 3:25 PM, John McCall <rjmccall at apple.com> wrote:
>
>> 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/20130208/a78d313a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smarter_implicit_copies_3.patch
Type: application/octet-stream
Size: 26952 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130208/a78d313a/attachment.obj>