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

[Duncan P. N. Exon Smith]

> On 2014-Dec-04, at 15:44, Alexey Samsonov <vonosmas at gmail.com> wrote:
> 
> FYI, StringMapEntry<>::GetStringMapEntryFromValue is inherently broken - I've failed to fix it in r223402, but failed. We can't just "get the offset" of a non-standard-layout object field.

Crap.

> I'd be happy to delete GetStringMapEntryFromValue from the codebase completely, but currently MDString::getString() is its only user.
> 

I'll remove that use, and kill the function myself.  Thanks.

> On Wed, Nov 19, 2014 at 12:10 AM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> On Tue, Nov 18, 2014 at 1:38 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > 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.
> 
> Pretty sure I remember introducing it, so I've only myself to blame - and probably somewhere in the last year or so. But I could be completely off on both counts.
>  
> 
> > , 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.
> 
> Adding this:
> 
> diff --git include/llvm/ADT/StringMap.h include/llvm/ADT/StringMap.h
> index 2feb2ab..99efcdd 100644
> --- include/llvm/ADT/StringMap.h
> +++ include/llvm/ADT/StringMap.h
> @@ -140,8 +140,9 @@ public:
>    /// Create - Create a StringMapEntry for the specified key and default
>    /// construct the value.
>    template <typename AllocatorTy, typename InitType>
> -  static StringMapEntry *Create(StringRef Key, AllocatorTy &Allocator,
> -                                InitType &&InitVal) {
> +  static typename std::enable_if<std::is_convertible<InitType, ValueTy>::value,
> +                                 StringMapEntry *>::type
> +  Create(StringRef Key, AllocatorTy &Allocator, InitType &&InitVal) {
>      unsigned KeyLength = Key.size();
>  
>      // Allocate a new item with space for the string at the end and a null
> @@ -169,8 +170,10 @@ public:
>    }
>  
>    /// Create - Create a StringMapEntry with normal malloc/free.
> -  template<typename InitType>
> -  static StringMapEntry *Create(StringRef Key, InitType &&InitVal) {
> +  template <typename InitType>
> +  static typename std::enable_if<std::is_convertible<InitType, ValueTy>::value,
> +                                 StringMapEntry *>::type
> +  Create(StringRef Key, InitType &&InitVal) {
>      MallocAllocator A;
>      return Create(Key, A, std::forward<InitType>(InitVal));
>    }
> 
> Only fails to compile on your new usage, not any other existing usage. (& now I'll have to try switching back to ValueTy directly to see what breaks there & why the InitType was added in there at all)
> 
>  
> 
> > 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.
> 
> Pretty public implementation details - though I do wonder if we could make them more private...  
> 
> One other simple possibility is to make the type Movable (could be privately movable - keeping that friending of StringMapEntry around - as much as dislike that too (limits StringMap's ability to be refactored without breaking this code - depends on exactly where the ctor is called from, etc)) and just pass in a Metadata(Context) - it looks like a fair few other StringMap uses do that or similar (many of them insert a default value then update the value to point back to the key data, in fact - I guess they could all be updated to use the technique you've got here, computing the entry address from that of the value).
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 
> 
> 
> -- 
> Alexey Samsonov
> vonosmas at gmail.com