<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Mar 22, 2016 at 9:17 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@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"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Tue, Mar 22, 2016 at 9:02 AM, Mehdi AMINI via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></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="color:rgb(0,0,0);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="color:rgb(0,0,0);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,0);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">Args</span><span style="color:rgb(0,0,0);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="color:rgb(0,0,0);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" 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="color:rgb(0,0,0);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="color:rgb(0,0,0);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="color:rgb(0,0,0);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="color:rgb(0,0,0);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,0);font-family:DejaVuSansMono,'DejaVu Sans Mono',courier,monospace;font-size:12.8px;white-space:nowrap">args</span><span style="color:rgb(0,0,0);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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br> </div><span class=""><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 class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span class=""><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><br>
</div></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>