[llvm] r211525 - Recommit 211309 (StringMap::insert), reverted in 211328 due to issues with private, but non-deleted, move members.
David Blaikie
dblaikie at gmail.com
Mon Jun 23 11:28:53 PDT 2014
Author: dblaikie
Date: Mon Jun 23 13:28:53 2014
New Revision: 211525
URL: http://llvm.org/viewvc/llvm-project?rev=211525&view=rev
Log:
Recommit 211309 (StringMap::insert), reverted in 211328 due to issues with private, but non-deleted, move members.
Certain versions of GCC (~4.7) couldn't handle the SFINAE on access
control, but with "= delete" (hidden behind a macro for portability)
this issue is worked around/addressed.
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=211525&r1=211524&r2=211525&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/StringMap.h (original)
+++ llvm/trunk/include/llvm/ADT/StringMap.h Mon Jun 23 13:28:53 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=211525&r1=211524&r2=211525&view=diff
==============================================================================
--- llvm/trunk/lib/Support/StringMap.cpp (original)
+++ llvm/trunk/lib/Support/StringMap.cpp Mon Jun 23 13:28:53 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=211525&r1=211524&r2=211525&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
+++ llvm/trunk/unittests/ADT/StringMapTest.cpp Mon Jun 23 13:28:53 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) {}
@@ -228,8 +266,8 @@ struct MoveOnly {
}
private:
- MoveOnly(const MoveOnly &);
- MoveOnly &operator=(const MoveOnly &);
+ MoveOnly(const MoveOnly &) LLVM_DELETED_FUNCTION;
+ MoveOnly &operator=(const MoveOnly &) LLVM_DELETED_FUNCTION;
};
TEST_F(StringMapTest, MoveOnlyKey) {
More information about the llvm-commits
mailing list