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

[Duncan P. N. Exon Smith]

> On 2014 Nov 18, at 12:59, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Tue, Nov 18, 2014 at 12:54 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > 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).
> 
> Not quite sure I agree - but we all make different tradeoffs along this spectrum, certainly.
>  
> > 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.
> 
> If I'm the one who cares about it enough to desire change & other people don't (not like anyone else is speaking up here), perhaps it is.
>  
> >> 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).
> 
> At least that would keep the weirdness out of a more common API.
>  
> 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.
> 
> Fair point - don't know how long

Frankly, I don't know how long either; I didn't check the log.

> , or if anyone relies on it (I'll try adding some SFINAE & see if anyone else is relying on it - in that case, adding your use would be more problematic even if the API has been able to do this for a while & we just haven't grown such uses) 

Okay, let me know what you figure out.

> 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.
> 
> Lots of things go that way to be disregarded essentially indefinitely, it's not something I have high hope will actually be addressed, going on past experience. For narrowly provided weirdness that's one thing, for common API that may grow new uses, etc, I'm more concerned about doing that.

FWIW, the original API change to StringMapEntry was pretty narrow,
since anyone creating from that directly is already messing with
implementation details.