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

[David Blaikie]

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.

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.


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