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

[David Blaikie]

On Mon, Dec 15, 2014 at 11:27 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
>
> > 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;


How so? The only difference, so far as I unedrstand it is in storage
efficiency - what makes it more flexible/useful?


> in other ways, it's worse off.


So far as I can tell, only in the sense that first has to be a function
instead of a member of the pair, and that it's got a specific key type,
rather than a type parameterized key type - but the C++ standard concepts
don't only apply to templates. Concrete classes can implement them equally
well, and I think StringMap does that with the only exception being the ()
on first.


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