<div dir="ltr"><br><div>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.</div>
<div><br></div><div>Mind if I commit?</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Feb 7, 2013 at 3:28 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Sure thing. I'm working on an update at the moment - I'll roll this in.<div><br></div><div>Cheers,</div>
<div>Lang.</div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Feb 7, 2013 at 3:25 PM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div>On Feb 5, 2013, at 6:45 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">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">
<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><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><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><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><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>
<span><font color="#888888"><div><br></div><div>John.</div></font></span></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>