[llvm] r221958 - StringMap: Test and finish off supporting perfectly forwarded values in StringMap operations.

[Duncan P. N. Exon Smith]

> On 2014-Nov-17, at 11:26, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Thu, Nov 13, 2014 at 4:51 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2014-Nov-13, at 16:41, David Blaikie <dblaikie at gmail.com> wrote:
> >
> > Author: dblaikie
> > Date: Thu Nov 13 18:41:46 2014
> > New Revision: 221958
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=221958&view=rev
> > Log:
> > StringMap: Test and finish off supporting perfectly forwarded values in StringMap operations.
> >
> > Followup to r221946.
> 
> I'm not convinced this is necessary when there are in-tree users,
> but I suppose it can't hurt anything.
> 
> Easier to iterate when someone goes to refactor it & ensures the codepaths continue to be tested even if a certain use case later gets refactored to not use that feature anymore.
>  
> 
> >
> > Modified:
> >    llvm/trunk/include/llvm/ADT/StringMap.h
> >    llvm/trunk/unittests/ADT/StringMapTest.cpp
> >
> > Modified: llvm/trunk/include/llvm/ADT/StringMap.h
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=221958&r1=221957&r2=221958&view=diff
> > ==============================================================================
> > --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
> > +++ llvm/trunk/include/llvm/ADT/StringMap.h Thu Nov 13 18:41:46 2014
> > @@ -170,9 +170,9 @@ public:
> >
> >   /// Create - Create a StringMapEntry with normal malloc/free.
> >   template<typename InitType>
> > -  static StringMapEntry *Create(StringRef Key, InitType InitVal) {
> > +  static StringMapEntry *Create(StringRef Key, InitType &&InitVal) {
> >     MallocAllocator A;
> > -    return Create(Key, A, std::move(InitVal));
> > +    return Create(Key, A, std::forward<InitType>(InitVal));
> >   }
> 
> This hardly adds new functionality.  The lvalue version collides with the
> `Create(StringRef, AllocatorTy&)` overload. 
> 
> Which no one is using? (or I would've just made the code fail to compile? depending on the exact nature of the collision) Again, why I love having tests - if it's required functionality, I should've broken something.
>  

Right, but you've extended functionality from my prior commit, and
added tests for what you're guessing my purpose was.  It's a rather
awkward way for us to co-develop ;).

I still think a test that "this API will compile" isn't useful.  It
permanently adds time to everyone's build, and doesn't catch any
regressions (at least, none that aren't caught by other in-tree
users failing to compile).

It would have saved us both time if I hadn't broken the commit up
into two, so perhaps I should just collapse them in the future when
I think a unit test is harmful.

> This will *only* add
> functionality for a temporary being passed in.  (See my comment below.)
> 
> A workaround would be to invert the `StringRef` and `AllocatorTy` args,
> but really you should always be passing the map's allocator.
> 
> Really? Lots of them use the default, so far as I can see.
>  
> Maybe this overload should be deleted entirely?
> 
> Maybe - but it seems it's used more than the other.
>  

I must have had a `grep`-fail :(.  It looked to me like there were
almost no users of this, most users went through the `StringMap`.

> 
> >
> >   static StringMapEntry *Create(StringRef Key) {
> > @@ -367,8 +367,9 @@ public:
> >   /// exists, return it.  Otherwise, default construct a value, insert it, and
> >   /// return.
> >   template <typename InitTy>
> > -  MapEntryTy &GetOrCreateValue(StringRef Key, InitTy Val) {
> > -    return *insert(std::make_pair(Key, std::move(Val))).first;
> > +  MapEntryTy &GetOrCreateValue(StringRef Key, InitTy &&Val) {
> > +    return *insert(std::pair<StringRef, ValueTy>(
> > +                       Key, std::forward<InitTy>(Val))).first;
> >   }
> >
> >   MapEntryTy &GetOrCreateValue(StringRef Key) {
> >
> > Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=221958&r1=221957&r2=221958&view=diff
> > ==============================================================================
> > --- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
> > +++ llvm/trunk/unittests/ADT/StringMapTest.cpp Thu Nov 13 18:41:46 2014
> > @@ -256,9 +256,15 @@ TEST_F(StringMapTest, NonDefaultConstruc
> >   ASSERT_EQ(iter->second.i, 123);
> > }
> >
> > +struct Immovable {
> > +  Immovable() {}
> > +  Immovable(Immovable&&) LLVM_DELETED_FUNCTION; // will disable the other special members
> > +};
> > +
> > struct MoveOnly {
> >   int i;
> >   MoveOnly(int i) : i(i) {}
> > +  MoveOnly(const Immovable&) : i(0) {}
> >   MoveOnly(MoveOnly &&RHS) : i(RHS.i) {}
> >   MoveOnly &operator=(MoveOnly &&RHS) {
> >     i = RHS.i;
> > @@ -270,7 +276,7 @@ private:
> >   MoveOnly &operator=(const MoveOnly &) LLVM_DELETED_FUNCTION;
> > };
> >
> > -TEST_F(StringMapTest, MoveOnlyKey) {
> > +TEST_F(StringMapTest, MoveOnly) {
> >   StringMap<MoveOnly> t;
> >   t.GetOrCreateValue("Test", MoveOnly(42));
> >   StringRef Key = "Test";
> > @@ -278,6 +284,14 @@ TEST_F(StringMapTest, MoveOnlyKey) {
> >       ->Destroy();
> > }
> >
> > +TEST_F(StringMapTest, CtorArg) {
> > +  StringMap<MoveOnly> t;
> > +  t.GetOrCreateValue("Test", Immovable());
> > +  StringRef Key = "Test";
> > +  StringMapEntry<MoveOnly>::Create(Key, Immovable())
> > +      ->Destroy();
> 
> In particular, this test would fail:
> 
>     Immovable I;
>     StringMapEntry<MoveOnly>::Create(Key, I)
> 
> Agreed.
> 
> Though on further reflection, I'm not sure this whole line of changes is the right direction - will discuss more on your use-case commit
>  
> 
> > +}
> > +
> > TEST_F(StringMapTest, MoveConstruct) {
> >   StringMap<int> A;
> >   A.GetOrCreateValue("x", 42);
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
>