<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hey David<div class=""><br class=""></div><div class="">How does this look for the move constructor/assignment operator? I basically punted all of the actual work to just moving the members themselves, and trust that they have suitably defined move operators.</div><div class=""><br class=""></div><div class="">I’ll send out updated patches once we have something tenable, but didn’t want to throw this in to a later patch with tests until it looks good on its own.</div><div class=""><br class=""></div><div class="">Also, i’m happy to put a “Required because of MSVC’ type comment above these if you want it to be clear why they aren’t just ‘= default’.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">Pete</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> ValueMap(<span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">ValueMap</span> &&other)</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> : <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Map</span>(<span style="font-variant-ligatures: no-common-ligatures; color: #703daa" class="">std</span>::move(other.Map)), <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">MDMap</span>(<span style="font-variant-ligatures: no-common-ligatures; color: #703daa" class="">std</span>::move(other.MDMap)),</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Data</span>(<span style="font-variant-ligatures: no-common-ligatures; color: #703daa" class="">std</span>::move(other.Data)) {</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> }</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">ValueMap</span>& <span style="font-variant-ligatures: no-common-ligatures; color: #bb2ca2" class="">operator</span>=(<span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">ValueMap</span> &&other) {</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Map</span> = <span style="font-variant-ligatures: no-common-ligatures; color: #703daa" class="">std</span>::move(other.Map);</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">MDMap</span> = <span style="font-variant-ligatures: no-common-ligatures; color: #703daa" class="">std</span>::move(other.MDMap);</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> <span style="font-variant-ligatures: no-common-ligatures; color: #4f8187" class="">Data</span> = std::move(other.Data);</div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo; color: rgb(187, 44, 162);" class=""><span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> </span>return<span style="font-variant-ligatures: no-common-ligatures; color: #000000" class=""> *</span>this<span style="font-variant-ligatures: no-common-ligatures; color: #000000" class="">;</span></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""> }</div></div><div style="margin: 0px; font-size: 11px; line-height: normal; font-family: Menlo;" class=""><br class=""></div><div class=""><div><blockquote type="cite" class=""><div class="">On Apr 27, 2015, at 10:41 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><br class=""><blockquote type="cite" class="">On Apr 27, 2015, at 10:37 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" class="">dblaikie@gmail.com</a>> wrote:<br class=""><br class="">Can we just drop all the special members from this type? (Rule of Zero)<br class=""></blockquote>That would be even better.<br class=""><blockquote type="cite" class=""><br class="">Since it has a unique_ptr member it'll be move-only anyway.<br class=""><br class="">(oh, except for MSVC, which doesn't synthesize move members - I don't<br class="">think it even works with explicit "= default" (check teh codebase to<br class="">see if there are any of those already - if there are, perhaps I'm<br class="">wrong?) so you might have to write them out explicitly)<br class=""></blockquote>I did a check for ‘)= default’ and found some hits so figured MSVC was ok. But now looking again I see those are only for C++03 constructors, destructors, and assignment operators. I bet you’re right that MSVC will still struggle with this.<br class=""><br class="">I’ll prepare another patch which should be MSVC safe.<br class=""><blockquote type="cite" class=""><br class="">Also: I'd drop the printfs from the test case - if they values are<br class="">interesting, just have the appropriate unit test check/EXPECT/whatever<br class="">for them. Then they're self-documenting without having to print them.<br class="">I can see, statically, in the code what the values are that they will<br class="">print.<br class=""></blockquote>Thats fine. You’re right about the values being self-documenting anyway.<br class=""><br class="">Cheers,<br class="">Pete<br class=""><blockquote type="cite" class=""><br class="">On Mon, Apr 27, 2015 at 9:59 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" class="">peter_cooper@apple.com</a>> wrote:<br class=""><blockquote type="cite" class="">Hi Rafael, David<br class=""><br class="">In<br class=""><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/273199.html" class="">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/273199.html</a>,<br class="">Rafael suggested returning a ValueMap from CloneFunction.<br class=""><br class="">Turns out that ValueMap has a deleted copy constructor, and no move<br class="">constructor all. This patch adds the move constructor (and assignment<br class="">operator). The default implementations of both of these seem to be fine.<br class=""><br class="">To test this, i managed to get the Constructible class from the SmallVector<br class="">unit test to wrap a Value*, then use that to count all the times the<br class="">ValueMap does any of the operators. I then went through the ValueMap and<br class="">DenseMap code to verify each of the values i’m checking against. I left<br class="">some printf’s in an #if 0 for future use in case we want to easily check the<br class="">values coming out of this code.<br class=""><br class="">Cheers,<br class="">Pete<br class=""><br class=""><br class=""><br class=""></blockquote></blockquote><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@cs.uiuc.edu" class="">llvm-commits@cs.uiuc.edu</a><br class="">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br class=""></div></blockquote></div><br class=""></div></body></html>