[llvm] r211309 - Add StringMap::insert(pair) consistent with the standard associative container concept.

[Rafael Espíndola]

Looks like this broke the build with gcc 4.7 in some bots and I reverted it.

On 19 June 2014 16:08, David Blaikie <dblaikie at gmail.com> wrote:
> Author: dblaikie
> Date: Thu Jun 19 15:08:56 2014
> New Revision: 211309
>
> URL: http://llvm.org/viewvc/llvm-project?rev=211309&view=rev
> Log:
> Add StringMap::insert(pair) consistent with the standard associative container concept.
>
> Patch by Agustín Bergé.
>
> Modified:
>     llvm/trunk/include/llvm/ADT/StringMap.h
>     llvm/trunk/lib/Support/StringMap.cpp
>     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=211309&r1=211308&r2=211309&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
> +++ llvm/trunk/include/llvm/ADT/StringMap.h Thu Jun 19 15:08:56 2014
> @@ -64,7 +64,7 @@ protected:
>    }
>
>    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 @@ public:
>      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 place, and the iterator component of
> +  /// 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 @@ public:
>    /// 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) {
>
> Modified: llvm/trunk/lib/Support/StringMap.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/StringMap.cpp?rev=211309&r1=211308&r2=211309&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Support/StringMap.cpp (original)
> +++ llvm/trunk/lib/Support/StringMap.cpp Thu Jun 19 15:08:56 2014
> @@ -181,7 +181,7 @@ StringMapEntryBase *StringMapImpl::Remov
>
>  /// 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 @@ void StringMapImpl::RehashTable() {
>    } 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 @@ void StringMapImpl::RehashTable() {
>        if (!NewTableArray[NewBucket]) {
>          NewTableArray[FullHash & (NewSize-1)] = Bucket;
>          NewHashArray[FullHash & (NewSize-1)] = FullHash;
> +        if (I == BucketNo)
> +          NewBucketNo = NewBucket;
>          continue;
>        }
>
> @@ -227,6 +230,8 @@ void StringMapImpl::RehashTable() {
>        // Finally found a slot.  Fill it in.
>        NewTableArray[NewBucket] = Bucket;
>        NewHashArray[NewBucket] = FullHash;
> +      if (I == BucketNo)
> +        NewBucketNo = NewBucket;
>      }
>    }
>
> @@ -235,4 +240,5 @@ void StringMapImpl::RehashTable() {
>    TheTable = NewTableArray;
>    NumBuckets = NewSize;
>    NumTombstones = 0;
> +  return NewBucketNo;
>  }
>
> Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=211309&r1=211308&r2=211309&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/StringMapTest.cpp Thu Jun 19 15:08:56 2014
> @@ -10,6 +10,7 @@
>  #include "gtest/gtest.h"
>  #include "llvm/ADT/StringMap.h"
>  #include "llvm/Support/DataTypes.h"
> +#include <tuple>
>  using namespace llvm;
>
>  namespace {
> @@ -203,6 +204,43 @@ TEST_F(StringMapTest, InsertTest) {
>    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(1u, t.getNumBuckets());
> +
> +  StringMap<uint32_t>::iterator It =
> +    t.insert(std::make_pair("abcdef", 42)).first;
> +  EXPECT_EQ(2u, t.getNumBuckets());
> +  EXPECT_EQ("abcdef", It->first());
> +  EXPECT_EQ(42u, It->second);
> +}
> +
>  // Create a non-default constructable value
>  struct StringMapTestStruct {
>    StringMapTestStruct(int i) : i(i) {}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits