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 <rjmcc...@apple.com> wrote: > On Feb 5, 2013, at 6:45 PM, Richard Smith <rich...@metafoo.co.uk> wrote: > > On Tue, Feb 5, 2013 at 5:31 PM, Lang Hames <lha...@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. >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits