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

[Duncan P. N. Exon Smith]

> 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.

> 
> 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.  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.

Maybe this overload should be deleted entirely?

> 
>   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)

> +}
> +
> 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