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

[David Blaikie]

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?


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


>
> >
> > (and I wouldn't mind if this function returned a reference instead of a
> pointer, but I realize that's just my own aesthetic preference and a lot of
> work to make the change that might not be better)
>
> Personally, I agree with your aesthetic here, but the style with IR is to
> return a pointer, so it's better to be consistent.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141210/4385c8c6/attachment.html>