<div dir="ltr">+1 to pretty much everything Duncan said.<br><br>But if we only so far need this in one place, and adding map-equivalent emplace is too much work for that, then maybe it's easier to just make the StringMap::Create have an option that takes varargs for the second param. Honestly emplace_second would probably be about as good/bad/etc.<br><br>Though I'm not sure it's all that much hassle to just do emplace proper?</div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Mar 6, 2016 at 5:38 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">"emplace" is actively misleading, because this doesn't actually do the<br>
same thing that "emplace" does in other pair/map-like structures.<br>
<br>
I chose "emplace_second" because the method emplaces the pair::second<br>
element; but if you don't like the name, I suspect David still doesn't<br>
like the semantics; std::piecewise_construct might be the best<br>
approach.<br>
<div class="HOEnZb"><div class="h5"><br>
> On 2016-Mar-06, at 17:14, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
><br>
> Isn't "_second" weird for a variadic template?<br>
> I mean you can call it:<br>
><br>
> auto I = Store.emplace_second(Key, arg1, arg2, arg3);<br>
><br>
> --<br>
> Mehdi<br>
><br>
>> On Mar 6, 2016, at 5:03 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
>><br>
>> +dblaikie<br>
>><br>
>> If you rename the method `emplace_second` (since you're only emplacing the<br>
>> second element, not the full value), then I'm happy with this.  However,<br>
>> David didn't like this patch when I proposed it a year or so ago.<br>
>><br>
>> IIRC, at the time David wanted a std::pair-style `emplace` in<br>
>> StringMapEntry complete with support for std::piecewise_construct_t.  Then<br>
>> you could call:<br>
>> --<br>
>>   auto I = Store.emplace(std::piecewise_construct,<br>
>>                          std::make_tuple(Str),<br>
>>                          std::make_tuple());<br>
>> --<br>
>><br>
>> I still think `emplace_second` is sufficiently clear since StringMap is so<br>
>> specialized (and the above looks like boilerplate to me), but this would<br>
>> be an uncontentious path forward.<br>
>><br>
>><br>
>>> On 2016-Mar-06, at 14:28, Mehdi AMINI <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>> wrote:<br>
>>><br>
>>> joker.eph created this revision.<br>
>>> joker.eph added a reviewer: dexonsmith.<br>
>>> joker.eph added a subscriber: llvm-commits.<br>
>>><br>
>>> Loading IR with debug info improves MDString::get() from 19ms to 10ms.<br>
>>> This is a rework of D16597 with adding an "emplace" method on the StringMap<br>
>>> to avoid requiring the MDString move ctor to be public.<br>
>>><br>
>>> <a href="http://reviews.llvm.org/D17920" rel="noreferrer" target="_blank">http://reviews.llvm.org/D17920</a><br>
>>><br>
>>> Files:<br>
>>> include/llvm/ADT/StringMap.h<br>
>>> include/llvm/IR/Metadata.h<br>
>>> lib/IR/Metadata.cpp<br>
>>><br>
>>> <D17920.49925.patch><br>
>><br>
><br>
<br>
</div></div></blockquote></div><br></div>