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

[Duncan P. N. Exon Smith]

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

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