<html><head><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Feb 5, 2013, at 6:45 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:</div><blockquote type="cite">On Tue, Feb 5, 2013 at 5:31 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto; ">
<div dir="ltr">Hi John, Richard,<div><br></div><div>Here's the updated patch for smarter copy-assignment/copy-construction for structs with Non-POD members.</div><div><br></div><div>Richard - I've added test cases for volatile fields, inner classes, array members, pod-like members, bitfields and volatile fields.</div>
<div>I've also pulled the naming scheme in line with the coding standards. I made sure I use ' &' rather than '& ' where appropriate.</div><div><br></div><div>John -</div><div>
<br></div><div class="gmail_extra">1) Qualifiers are now being grabbed once. <br>2) I now fall back on existing codegen for when GCMode != NonGC.</div><div class="gmail_extra">3) Made sure I round-up to the nearest byte when copying bitfields.<br>
</div><div class="gmail_extra">4) I've tried to address the style issues you raised.</div><div class="gmail_extra">5)</div><div class="im"><div class="gmail_extra"><br></div><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> + // Bail out on non-POD, not-trivially-constructable members.</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> + if (!FieldType.isPODType(GetCGF()</span><span style="color:rgb(80,0,80)">.getContext()) &&</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> + (!CE || !CE->getConstructor()-></span><span style="color:rgb(80,0,80)">isTrivial()))</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> + return false;</span><br style="color:rgb(80,0,80)">> <br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> If you're calling a trivial copy/move constructor, it does not matter</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> whether the type is POD. Test case: give a field a type with a trivial</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> copy constructor and non-trivial destructor, or more realistically a</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> non-trivial default constructor.</span><br style="color:rgb(80,0,80)"><div class="gmail_extra"><br></div></div><div class="gmail_extra">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.</div>
</div></blockquote><div><br></div><div>Just because the type is POD, doesn't mean the selected constructor is trivial.</div></div></blockquote><div><br></div><div>The point is that Lang's code was gating on *both*, which is clearly</div><div>unnecessary in the case of class types.</div><div><br></div><blockquote type="cite"><div class="gmail_quote"><div>Counterexample:</div><div><br></div><div>struct S { S() = default; template<typename T> S(T&&); S(const S&) = default; };</div>
<div>struct U { U(); S s; };</div><div>static_assert(__is_pod(S), "");</div><div>U u = static_cast<U&&>(U()); // static_cast to disable copy elision</div><div><br></div><div>S is POD, but U's move constructor calls S's constructor template.</div>
<div><br></div><div>I think this is what you mean:</div><div><br></div><div> CE ? CE->getConstructor()->isTrivial() : FieldType.isTriviallyCopyableType(Ctx)</div></div></blockquote><div><br></div><div>Mmm. This would actually drop members of reference type, which really can be safely memcpy'ed in a copy constructor.</div><div><br></div><div>On the other than, it would also drop members of atomic type, which really shouldn't be memcpy'ed. So there's that.</div><div><br></div><div>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.</div><div><br></div><div>John.</div></div></body></html>