[llvm] r223806 - Fix a GCC build failure from r223802

[Duncan P. N. Exon Smith]

> On 2014 Dec 11, at 10:15, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Wed, Dec 10, 2014 at 11:50 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2014 Dec 10, at 10:36, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Wed, Dec 10, 2014 at 10:22 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> >
> > > On 2014 Dec 9, at 11:02, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > >
> > >
> > > On Tue, Dec 9, 2014 at 10:52 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > > Author: dexonsmith
> > > Date: Tue Dec  9 12:52:38 2014
> > > New Revision: 223806
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=223806&view=rev
> > > Log:
> > > Fix a GCC build failure from r223802
> > >
> > > Modified:
> > >     llvm/trunk/lib/IR/Metadata.cpp
> > >
> > > Modified: llvm/trunk/lib/IR/Metadata.cpp
> > > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=223806&r1=223805&r2=223806&view=diff
> > > ==============================================================================
> > > --- llvm/trunk/lib/IR/Metadata.cpp (original)
> > > +++ llvm/trunk/lib/IR/Metadata.cpp Tue Dec  9 12:52:38 2014
> > > @@ -341,7 +341,8 @@ MDString *MDString::get(LLVMContext &Con
> > >    if (I != Store.end())
> > >      return &I->second;
> > >
> > > -  auto *Entry = StringMapEntry<MDString>::Create(Str, Store.getAllocator());
> > > +  auto *Entry =
> > > +      StringMapEntry<MDString>::Create(Str, Store.getAllocator(), MDString());
> > >
> > > Looks like this whole function could maybe be a bit simpler (& avoid the double lookup on first access, as well as the raw StringMapEntry<>::Create):
> > >
> > > auto &IterBool = Store.insert(std::make_pair(Str, MDString()));
> > > if (IterBool.second)
> > >   IterBool.first->second.Entry = &*IterBool.first;
> > > return &IterBool.first->second;
> > >
> > > (I'm not even sure it's worth the conditional? Maybe the unconditional assignment is cheaper?)
> > >
> > > (at some point I think we might be able to avoid exposing the ability to manually create/insert/etc StringMapEntries & pretend, as much as possible, that they're just a pair like a normal map)
> >
> > I agree that flow is better.
> >
> > However, I just implemented it, and it looks to me like it requires a
> > copy constructor, as well the unfortunate:
> >
> >     friend struct std::pair<StringRef, MDString>;
> >
> > I'm confused then - doesn't the above code:
> >
> >  +  auto *Entry =
> >  +      StringMapEntry<MDString>::Create(Str, Store.getAllocator(), MDString());
> >
> > require a copy (or move) ctor in MDString too?
> 
> Yes, but `std::pair<>` is widely used and general purpose
> 
> In particular, I don't think think users are likely to accidentally make a
> `StringMapEntry<StringRef, MDString>` instead of
> `StringMapEntry<StringRef, MDString *>`.  (But to be honest, I wasn't
> super happy with this either.)
> 
> I suppose `std::pair<StringRef, MDString *>` is an unlikely thing to want
> too, so maybe I'm being too conservative.
> 
> >
> > I don't think this is acceptable.  It's really not valid to copy or move
> > an `MDString` outside of `MDString::get()`, so it's important that
> > creating a pair out of one is a compile-time error.
> >
> > This could be fixed by adding new API to `StringMap` that avoids the use
> > of `std::pair<>`, but I'm not sure it fits with your vision there.
> >
> > Nevertheless, here's the API that I was thinking.  In `StringMap`:
> >
> >     template <class ArgsTy...>
> >     std::pair<iterator, bool> emplace_second(StringRef, ArgsTy &&...);
> >
> > This emplaces the `mapped_type` with the provided args (open to
> > suggestions on naming).  IMO, this matches `StringMap`'s
> > specialization around the `key_type` being `StringRef`.
> >
> > Thoughts?
> >
> > Ah, coming back around to the discussion from a few weeks back.
> 
> :)
> 
> > I'd prefer the actual STL-compatible API of 'emplace()' rather than special-casing the second parameter, of course, though I realize it's a tad more work to wrangle the tuples, etc. (wouldn't mind special casing "piecewise_construct, StringRef, tuple" which would be similar in functionality to your API (but would require tuple wrangling) without having to deal with the full generality of being able to pass the StringRef ctor argument tuple, etc, for limited benefit.
> 
> So this would match the STL API more closely, but I don't think the STL
> API is the right one for `StringMap`.  It seems like the `std::pair<>`
> idiom adds a lot of boiler-plate for not much benefit.
> 
> Since we already have a custom ADT, we have the opportunity to fit the
> use case better.
> 
> We do, but problems arise when we want to interoperate with templates that may operate on the common API. Granted, parts of StringMap can never be consistent with that API (just the "()" on entry.first, for the most part - and I'm not sure if the iterator invalidation semantics are baked into the associative container requirements of the standard), but keeping as much of it as possible seems generally good.
> 

If this becomes a real problem, we might add a wrapper API that makes it fit
into the templates.

> This goes for humans as well as templates - when the API looks sort of like a map and then it's weirdly different in some places (while still using the same terms, especially) that's going to create some friction at least for me (& I assume others) - switching back & forth between standard and almost-standard things. Whenever I hit this with the ADT libraries I try to fill in the holes/make the API consistent to reduce that friction.
> 

Honestly, I still think the custom API is way better for humans than shoving
the StringMap emplacement into the round hole of piecewise construction.

I see your point about terms potentially being confusing, but we could
choose a different term to solve that...

But even in the STL, the same terms are reused in different contexts, and
we adapt.  IMO, this is still emplacement, and using the same term (even for
a slightly different API) makes things clear, not muddy.

I don't think StringMap fits neatly into the "associative container" concept
in the STL -- in some ways, it's more useful/flexible; in other ways, it's
worse off.  But it should have an API that makes sense for its quirks.

> 
> With your concession, the users of the API would be doing one of:
> 
>     emplace(Str);
>     emplace(Str, A1);
>     emplace(std::piecewise_construct, Str, std::make_tuple(A1, A2));
>     emplace(std::piecewise_construct, Str, std::make_tuple(A1, A2, A3));
> 
> (Not that these could be passed directly to `StringMapEntry`, since it
> doesn't actually handle the `StringRef`.)
> 
> IMO, this is better API:
> 
>     emplace(Str);
>     emplace(Str, A1);
>     emplace(Str, A1, A2);
>     emplace(Str, A1, A2, A3);
> 
> In the general case -- where it's useful to piecewise construct
> `key_type` -- `std::piecewise_construct` is a benefit.  Here I think it
> just gets in the way of writing (and reading) the code.
> 
> That's aside from the latter being simpler to implement and maintain.
>