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 <[email protected]> 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 <[email protected]> wrote: > >> On Feb 5, 2013, at 6:45 PM, Richard Smith <[email protected]> wrote: >> >> On Tue, Feb 5, 2013 at 5:31 PM, Lang Hames <[email protected]> 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. >> > >
smarter_implicit_copies_3.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
