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

Duncan P. N. Exon Smith dexonsmith at apple.com
Wed Dec 10 11:50:39 PST 2014


> 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.

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.



More information about the llvm-commits mailing list