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

Thu Dec 4 15:44:39 PST 2014

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. I'd be happy to
delete GetStringMapEntryFromValue from the codebase completely, but
currently MDString::getString() is its only user.

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141204/6e331d3a/attachment.html>