<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Apr 28, 2015 at 11:42 AM, Pete Cooper <span dir="ltr"><<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@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">Hey David<div><br></div><div>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></blockquote><div><br>Yep, that's the right idea.<br> </div><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>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.<br></div><div><br></div><div>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></blockquote><div><br>I'm not too fussed about commenting it. We have so many of these already & will only have more. Eventually we'll switch to some version of MSVC that supports the defaults & we'll just remove all of them.<br> </div><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><br></div><div>Cheers,</div><div>Pete</div><div><br></div><div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo;min-height:13px"><br></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">  ValueMap(<span style="color:#4f8187">ValueMap</span> &&other)</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">      : <span style="color:#4f8187">Map</span>(<span style="color:#703daa">std</span>::move(other.Map)), <span style="color:#4f8187">MDMap</span>(<span style="color:#703daa">std</span>::move(other.MDMap)),</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">        <span style="color:#4f8187">Data</span>(<span style="color:#703daa">std</span>::move(other.Data)) {</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">  }</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">  <span style="color:#4f8187">ValueMap</span>& <span style="color:#bb2ca2">operator</span>=(<span style="color:#4f8187">ValueMap</span> &&other) {</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">    <span style="color:#4f8187">Map</span> = <span style="color:#703daa">std</span>::move(other.Map);</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">    <span style="color:#4f8187">MDMap</span> = <span style="color:#703daa">std</span>::move(other.MDMap);</div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">    <span style="color:#4f8187">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)"><span style="color:#000000">    </span>return<span style="color:#000000"> *</span>this<span style="color:#000000">;</span></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo">  }</div></div><div style="margin:0px;font-size:11px;line-height:normal;font-family:Menlo"><br></div><div><div><blockquote type="cite"><div><div class="h5"><div>On Apr 27, 2015, at 10:41 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>> wrote:</div><br></div></div><div><div><div class="h5"><br><blockquote type="cite">On Apr 27, 2015, at 10:37 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br><br>Can we just drop all the special members from this type? (Rule of Zero)<br></blockquote>That would be even better.<br><blockquote type="cite"><br>Since it has a unique_ptr member it'll be move-only anyway.<br><br>(oh, except for MSVC, which doesn't synthesize move members - I don't<br>think it even works with explicit "= default" (check teh codebase to<br>see if there are any of those already - if there are, perhaps I'm<br>wrong?) so you might have to write them out explicitly)<br></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><br>I’ll prepare another patch which should be MSVC safe.<br><blockquote type="cite"><br>Also: I'd drop the printfs from the test case - if they values are<br>interesting, just have the appropriate unit test check/EXPECT/whatever<br>for them. Then they're self-documenting without having to print them.<br>I can see, statically, in the code what the values are that they will<br>print.<br></blockquote>Thats fine.  You’re right about the values being self-documenting anyway.<br><br>Cheers,<br>Pete<br><blockquote type="cite"><br>On Mon, Apr 27, 2015 at 9:59 AM, Pete Cooper <<a href="mailto:peter_cooper@apple.com" target="_blank">peter_cooper@apple.com</a>> wrote:<br><blockquote type="cite">Hi Rafael, David<br><br>In<br><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/273199.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150420/273199.html</a>,<br>Rafael suggested returning a ValueMap from CloneFunction.<br><br>Turns out that ValueMap has a deleted copy constructor, and no move<br>constructor all.  This patch adds the move constructor (and assignment<br>operator). The default implementations of both of these seem to be fine.<br><br>To test this, i managed to get the Constructible class from the SmallVector<br>unit test to wrap a Value*, then use that to count all the times the<br>ValueMap does any of the operators.  I then went through the ValueMap and<br>DenseMap code to verify each of the values i’m checking against.  I left<br>some printf’s in an #if 0 for future use in case we want to easily check the<br>values coming out of this code.<br><br>Cheers,<br>Pete<br><br><br><br></blockquote></blockquote><br><br></div></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div></blockquote></div><br></div></div>