[PATCH] Add standard insert overloads to StringMap

[Alp Toker]

On 19/06/2014 22:37, Agustín Bergé wrote:
> Changes in this iteration:
>
> - Removed hash check.
> - Added comment explaining how brittle the rehash test case is.
>
> David: I don't have commit access, could you take care of that?
>
> http://reviews.llvm.org/D4153
>
> Files:
>    include/llvm/ADT/StringMap.h
>    lib/Support/StringMap.cpp
>    unittests/ADT/StringMapTest.cpp
>
> D4153.10652.patch
>
>
> Index: include/llvm/ADT/StringMap.h
> ===================================================================
> --- include/llvm/ADT/StringMap.h
> +++ include/llvm/ADT/StringMap.h
> @@ -64,7 +64,7 @@
>     }
>   
>     StringMapImpl(unsigned InitSize, unsigned ItemSize);
> -  void RehashTable();
> +  unsigned RehashTable(unsigned BucketNo = 0);
>   
>     /// LookupBucketFor - Look up the bucket that the specified string should end
>     /// up in.  If it already exists as a key in the map, the Item pointer for the
> @@ -323,6 +323,28 @@
>       return true;
>     }
>   
> +  /// insert - Inserts the specified key/value pair into the map if the key
> +  /// isn't already in the map. The bool component of the returned pair is true
> +  /// if and only if the insertion takes places, and the iterator component of

Takes 'place'

Looks okay otherwise.

Thanks!
Alp.

> +  /// the pair points to the element with key equivalent to the key of the pair.
> +  std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> KV) {
> +    unsigned BucketNo = LookupBucketFor(KV.first);
> +    StringMapEntryBase *&Bucket = TheTable[BucketNo];
> +    if (Bucket && Bucket != getTombstoneVal())
> +      return std::make_pair(iterator(TheTable + BucketNo, false),
> +                            false); // Already exists in map.
> +
> +    if (Bucket == getTombstoneVal())
> +      --NumTombstones;
> +    Bucket =
> +        MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));
> +    ++NumItems;
> +    assert(NumItems + NumTombstones <= NumBuckets);
> +
> +    BucketNo = RehashTable(BucketNo);
> +    return std::make_pair(iterator(TheTable + BucketNo, false), true);
> +  }
> +
>     // clear - Empties out the StringMap
>     void clear() {
>       if (empty()) return;
> @@ -346,24 +368,7 @@
>     /// return.
>     template <typename InitTy>
>     MapEntryTy &GetOrCreateValue(StringRef Key, InitTy Val) {
> -    unsigned BucketNo = LookupBucketFor(Key);
> -    StringMapEntryBase *&Bucket = TheTable[BucketNo];
> -    if (Bucket && Bucket != getTombstoneVal())
> -      return*static_cast<MapEntryTy*>(Bucket);
> -
> -    MapEntryTy *NewItem = MapEntryTy::Create(Key, Allocator, std::move(Val));
> -
> -    if (Bucket == getTombstoneVal())
> -      --NumTombstones;
> -    ++NumItems;
> -    assert(NumItems + NumTombstones <= NumBuckets);
> -
> -    // Fill in the bucket for the hash table.  The FullHashValue was already
> -    // filled in by LookupBucketFor.
> -    Bucket = NewItem;
> -
> -    RehashTable();
> -    return *NewItem;
> +    return *insert(std::make_pair(Key, std::move(Val))).first;
>     }
>   
>     MapEntryTy &GetOrCreateValue(StringRef Key) {
> Index: lib/Support/StringMap.cpp
> ===================================================================
> --- lib/Support/StringMap.cpp
> +++ lib/Support/StringMap.cpp
> @@ -181,7 +181,7 @@
>   
>   /// RehashTable - Grow the table, redistributing values into the buckets with
>   /// the appropriate mod-of-hashtable-size.
> -void StringMapImpl::RehashTable() {
> +unsigned StringMapImpl::RehashTable(unsigned BucketNo) {
>     unsigned NewSize;
>     unsigned *HashTable = (unsigned *)(TheTable + NumBuckets + 1);
>   
> @@ -193,9 +193,10 @@
>     } else if (NumBuckets-(NumItems+NumTombstones) <= NumBuckets/8) {
>       NewSize = NumBuckets;
>     } else {
> -    return;
> +    return BucketNo;
>     }
>   
> +  unsigned NewBucketNo = BucketNo;
>     // Allocate one extra bucket which will always be non-empty.  This allows the
>     // iterators to stop at end.
>     StringMapEntryBase **NewTableArray =
> @@ -215,6 +216,8 @@
>         if (!NewTableArray[NewBucket]) {
>           NewTableArray[FullHash & (NewSize-1)] = Bucket;
>           NewHashArray[FullHash & (NewSize-1)] = FullHash;
> +        if (I == BucketNo)
> +          NewBucketNo = NewBucket;
>           continue;
>         }
>         
> @@ -227,12 +230,15 @@
>         // Finally found a slot.  Fill it in.
>         NewTableArray[NewBucket] = Bucket;
>         NewHashArray[NewBucket] = FullHash;
> +      if (I == BucketNo)
> +        NewBucketNo = NewBucket;
>       }
>     }
>     
>     free(TheTable);
>     
>     TheTable = NewTableArray;
>     NumBuckets = NewSize;
>     NumTombstones = 0;
> +  return NewBucketNo;
>   }
> Index: unittests/ADT/StringMapTest.cpp
> ===================================================================
> --- unittests/ADT/StringMapTest.cpp
> +++ unittests/ADT/StringMapTest.cpp
> @@ -10,6 +10,7 @@
>   #include "gtest/gtest.h"
>   #include "llvm/ADT/StringMap.h"
>   #include "llvm/Support/DataTypes.h"
> +#include <tuple>
>   using namespace llvm;
>   
>   namespace {
> @@ -201,6 +202,43 @@
>             StringRef(testKeyFirst, testKeyLength),
>             testMap.getAllocator(), 1u));
>     assertSingleItemMap();
> +}
> +
> +// Test insert(pair<K, V>) method
> +TEST_F(StringMapTest, InsertPairTest) {
> +  bool Inserted;
> +  StringMap<uint32_t>::iterator NewIt;
> +  std::tie(NewIt, Inserted) =
> +      testMap.insert(std::make_pair(testKeyFirst, testValue));
> +  EXPECT_EQ(1u, testMap.size());
> +  EXPECT_EQ(testValue, testMap[testKeyFirst]);
> +  EXPECT_EQ(testKeyFirst, NewIt->first());
> +  EXPECT_EQ(testValue, NewIt->second);
> +  EXPECT_TRUE(Inserted);
> +
> +  StringMap<uint32_t>::iterator ExistingIt;
> +  std::tie(ExistingIt, Inserted) =
> +      testMap.insert(std::make_pair(testKeyFirst, testValue + 1));
> +  EXPECT_EQ(1u, testMap.size());
> +  EXPECT_EQ(testValue, testMap[testKeyFirst]);
> +  EXPECT_FALSE(Inserted);
> +  EXPECT_EQ(NewIt, ExistingIt);
> +}
> +
> +// Test insert(pair<K, V>) method when rehashing occurs
> +TEST_F(StringMapTest, InsertRehashingPairTest) {
> +  // Check that the correct iterator is returned when the inserted element is
> +  // moved to a different bucket during internal rehashing. This depends on
> +  // the particular key, and the implementation of StringMap and HashString.
> +  // Changes to those might result in this test not actually checking that.
> +  StringMap<uint32_t> t(1);
> +  EXPECT_EQ(1, t.getNumBuckets());
> +
> +  StringMap<uint32_t>::iterator It =
> +    t.insert(std::make_pair("abcdef", 42)).first;
> +  EXPECT_EQ(2, t.getNumBuckets());
> +  EXPECT_EQ("abcdef", It->first());
> +  EXPECT_EQ(42, It->second);
>   }
>   
>   // Create a non-default constructable value
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-- 
http://www.nuanti.com
the browser experts