[PATCH] D17920: Query the StringMap only once when creating MDString (NFC)

Duncan P. N. Exon Smith via llvm-commits llvm-commits at lists.llvm.org
Sun Mar 6 17:38:13 PST 2016


"emplace" is actively misleading, because this doesn't actually do the
same thing that "emplace" does in other pair/map-like structures.

I chose "emplace_second" because the method emplaces the pair::second
element; but if you don't like the name, I suspect David still doesn't
like the semantics; std::piecewise_construct might be the best
approach.

> On 2016-Mar-06, at 17:14, Mehdi Amini <mehdi.amini at apple.com> wrote:
> 
> Isn't "_second" weird for a variadic template? 
> I mean you can call it:
> 
> auto I = Store.emplace_second(Key, arg1, arg2, arg3);
> 
> -- 
> Mehdi
> 
>> On Mar 6, 2016, at 5:03 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> +dblaikie
>> 
>> If you rename the method `emplace_second` (since you're only emplacing the
>> second element, not the full value), then I'm happy with this.  However,
>> David didn't like this patch when I proposed it a year or so ago.
>> 
>> IIRC, at the time David wanted a std::pair-style `emplace` in
>> StringMapEntry complete with support for std::piecewise_construct_t.  Then
>> you could call:
>> --
>>   auto I = Store.emplace(std::piecewise_construct,
>>                          std::make_tuple(Str),
>>                          std::make_tuple());
>> --
>> 
>> I still think `emplace_second` is sufficiently clear since StringMap is so
>> specialized (and the above looks like boilerplate to me), but this would
>> be an uncontentious path forward.
>> 
>> 
>>> On 2016-Mar-06, at 14:28, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>>> 
>>> joker.eph created this revision.
>>> joker.eph added a reviewer: dexonsmith.
>>> joker.eph added a subscriber: llvm-commits.
>>> 
>>> Loading IR with debug info improves MDString::get() from 19ms to 10ms.
>>> This is a rework of D16597 with adding an "emplace" method on the StringMap
>>> to avoid requiring the MDString move ctor to be public.
>>> 
>>> http://reviews.llvm.org/D17920
>>> 
>>> Files:
>>> include/llvm/ADT/StringMap.h
>>> include/llvm/IR/Metadata.h
>>> lib/IR/Metadata.cpp
>>> 
>>> <D17920.49925.patch>
>> 
> 



More information about the llvm-commits mailing list