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

[David Blaikie]

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