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

[David Blaikie via llvm-commits]

+1 to pretty much everything Duncan said.

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.

Though I'm not sure it's all that much hassle to just do emplace proper?

On Sun, Mar 6, 2016 at 5:38 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> "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>
> >>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160307/a937f485/attachment.html>