[llvm] r221960 - IR: Rewrite uniquing and creation of MDString

[Duncan P. N. Exon Smith]

> On 2014 Nov 18, at 11:52, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Nov 18, 2014 at 11:19 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2014 Nov 18, at 10:06, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Unexposed, but not magic - it can be implemented in LLVM as it is in libc++. A few of these utilities are a bit of a pain to write, but they're generic enough not to need to be written very often.
> 
>> This is spiraling into a major project :).  I just want to pass in
>> a context by reference to prevent reference/pointer disagreement
>> in `MDString::get()` vs. `MDString::MDString()`, so I want some
>> API that is clear and correct.
> 
> I'm not sure it's so clear when this can invoke explicit ctors in a context where one would only really expect implicit conversions. Maybe for this use case it's not problematic, but for others this could be a pretty confusing stumbling block/bug.
>  
>> How about I file a PR for this?  Your argument that `emplace()`
>> would be a good abstraction here is convincing, but encapsulating
>> `StringMap` is pretty orthogonal to my current work.
> 
> If you don't have time to address this now,

I don't, really.  If I fix every strange API I come across, I'll
never complete the metadata-value-split (or the custom debug info
IR that motivated it).

> I'll find some & just do it - I don't really want this implicit invocation of explicit ctors sticking around.

But me being pragmatic shouldn't create work for you... that's the
wrong outcome.

>> In the meantime, I think the perfect-forwarding I added in the
>> 3-arg version of `StringMapEntry::Create()` is an incremental
>> improvement.
>> 
> I don't think this is an incremental improvement since it can lead to explicit ctors being called essentially implicitly.

I'd be happy to back out my API changes and allow the `MDString`
pointer/reference disagreement, but I don't think that will
satisfy you (although let me know if I'm wrong).

Explicit constructors could *already* be called implicitly, I just
changed it so that the argument (whether implicit or explicit)
could be passed by reference.  If this API is flawed (and I think
you're right: it is), it's been flawed for a long time.

Not that we shouldn't fix it; I just don't see why fixing it
should be urgent.  IMO, the only urgency is filing a PR (or adding
a FIXME) so that it's tracked somehow.