[llvm] r221960 - IR: Rewrite uniquing and creation of MDString
David Blaikie
dblaikie at gmail.com
Thu Dec 4 15:56:26 PST 2014
On Thu, Dec 4, 2014 at 3:44 PM, 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. I'd be happy to
> delete GetStringMapEntryFromValue from the codebase completely, but
> currently MDString::getString() is its only user.
>
FWIW when I was refactoring the StringMap API to be more consistent with
the associative container concept (removing GetOrCreate in favor of the
'insert', etc) I found several other uses that wanted/were doing similar
things to MDString, but paid the overhead of a StringRef in their value
that referenced back into the key.
>
> 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/b1707115/attachment.html>
More information about the llvm-commits
mailing list