<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 22, 2016 at 10:03 AM, Mehdi Amini <span dir="ltr"><<a href="mailto:mehdi.amini@apple.com" target="_blank">mehdi.amini@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"><br><div><span class=""><blockquote type="cite"><div>On Mar 22, 2016, at 9:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br><div><br><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Tue, Mar 22, 2016 at 9:17 AM, David Blaikie<span> </span><span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span><span> </span>wrote:<br><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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Mar 22, 2016 at 9:02 AM, Mehdi AMINI via llvm-commits<span> </span><span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span><span> </span>wrote:<br><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">joker.eph added a comment.<br><span><br>> std::unordered_map::emplace returns the classic <iterator, bool> pair. Perhaps emplace_second should do the same? Rather than relying on being able to detect the invalid default constructed state of the key?<br><br><br></span>I don't understand what you mean: `std::pair<iterator, bool> emplace_second(StringRef Key, ArgsTy &&... Args) `<br></blockquote></span><div><br>Right, to match/parallel with:<br><br><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,221);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">template</span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,128);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap"><</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap"> </span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,221);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">class</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap">... </span><span style="line-height:14.08px;margin:0px;padding:0px;font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">Args</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap"> </span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,128);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">></span><br style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap"><a href="http://en.cppreference.com/w/cpp/utility/pair" style="text-decoration:none;color:rgb(0,48,128);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap;background-image:none;background-repeat:initial initial" target="_blank"><span style="line-height:1.1em;margin:0px;padding:0px">std::<span style="line-height:1.1em;margin:0px;padding:0px">pair</span></span></a><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,128);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap"><</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap">iterator,</span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,255);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">bool</span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,128);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">></span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap"> emplace</span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,128,0);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">(</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap"> Args</span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,0,64);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">&&</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap">... </span><span style="line-height:14.08px;margin:0px;padding:0px;font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">args</span><span style="font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;line-height:14.08px;white-space:nowrap"> </span><span style="line-height:14.08px;margin:0px;padding:0px;color:rgb(0,128,0);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">)</span></div></div></div></div></blockquote><div><br></div><div>Sorry - my mistake, I see (as you pointed out on IRC) that emplace_second already returns the expected pair.<br><br>I'd just hastily misread the code in MDString::get (seeing the access of the value before if test and then misread/assumed the value was used in the if test, instead of the bool in the pair - not an uncommon idiom (when the value has an obvious default value that's not valid for the use case))<br></div><div> </div><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"><div class="gmail_extra"><div class="gmail_quote"><div><br> </div><span><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"><span><br><br>================<br>Comment at: include/llvm/IR/Metadata.h:595<br>@@ -594,3 +594,3 @@<br>   MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {}<br>-  MDString(MDString &&) : Metadata(MDStringKind, Uniqued) {}<br>+  MDString(MDString &&R) = default;<br><br>----------------<br></span><span>dblaikie wrote:<br>> Why do we need a move ctor at all if we're supporting emplace? Can we just delete it? (even if we omit it, it should be removed implicitly - same with the move assignment above, I think)<br></span>StringMap requires movable types (for std::pair I think).<br></blockquote><div><br></div></span><div>If we're supporting emplace, it's usually because the identity of the object is important from the moment of construction - so perhaps we should be making sure that identity is actually preserved, same as the standard emplace.<br></div></div></div></div></blockquote><div><br></div><div>Still interested in this ^</div></div></div></blockquote><div><br></div></span><div>So, the move ctor is need by the StringMap for other purpose than emplace_second().</div></div></div></blockquote><div><br></div><div>OK - here's where a unit test would be nice, perhaps, to demonstrate that emplace_second builds exactly one instance of the value and doesn't move it anywhere (and, hopefully, that it can be called without a movable value - but perhaps that's not possible? (growing may involve moving keys?))</div><div> </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><div> Here I fix the move ctor to be actually a move which it was not. </div><div>I could move this to a separate patch, the only reason it is here is that in the original implementation I had to make it public, and I moved it back to private but left the "implementation" change.</div></div></div></blockquote><div><br></div><div>*nod* Might be that DenseMap can't support non-movable keys, so it'll be a bit different from emplace on standard maps.</div><div> </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><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- </div><div>Mehdi</div></font></span><span class=""><div><br></div><div><br></div><div><br></div><br><blockquote type="cite"><div><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div> </div><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"><div class="gmail_extra"><div class="gmail_quote"><div></div><span><div> </div><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><div><br><br><br><a href="http://reviews.llvm.org/D17920" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17920</a><br><br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></div></blockquote></span></div></div></div></blockquote></div></div></blockquote></span></div><br></div></blockquote></div><br></div></div>