Index: include/llvm/ADT/StringMap.h =================================================================== --- include/llvm/ADT/StringMap.h +++ include/llvm/ADT/StringMap.h @@ -143,11 +143,11 @@ StringRef first() const { return StringRef(getKeyData(), getKeyLength()); } - /// Create - Create a StringMapEntry for the specified key and default - /// construct the value. - template + /// Create a StringMapEntry for the specified key construct the value using + /// \p InitiVals. + template static StringMapEntry *Create(StringRef Key, AllocatorTy &Allocator, - InitType &&InitVal) { + InitTypes &&... InitVals) { unsigned KeyLength = Key.size(); // Allocate a new item with space for the string at the end and a null @@ -159,8 +159,9 @@ StringMapEntry *NewItem = static_cast(Allocator.Allocate(AllocSize,Alignment)); - // Default construct the value. - new (NewItem) StringMapEntry(KeyLength, std::forward(InitVal)); + // Construct the value. + new (NewItem) + StringMapEntry(KeyLength, std::forward(InitVals)...); // Copy the string information. char *StrBuffer = const_cast(NewItem->getKeyData()); @@ -170,11 +171,6 @@ return NewItem; } - template - static StringMapEntry *Create(StringRef Key, AllocatorTy &Allocator) { - return Create(Key, Allocator, ValueTy()); - } - /// Create - Create a StringMapEntry with normal malloc/free. template static StringMapEntry *Create(StringRef Key, InitType &&InitVal) { @@ -296,8 +292,10 @@ return ValueTy(); } + /// Lookup the ValueTy for the \p Key, or create a default constructed value + /// if the key is not in the map. ValueTy &operator[](StringRef Key) { - return insert(std::make_pair(Key, ValueTy())).first->second; + return emplace_second(Key).first->second; } /// count - Return 1 if the element is in the map, 0 otherwise. @@ -329,7 +327,16 @@ /// 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 insert(std::pair KV) { - unsigned BucketNo = LookupBucketFor(KV.first); + return emplace_second(KV.first, std::move(KV.second)); + } + + /// Emplace a new element for the specified key 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. + template + std::pair emplace_second(StringRef Key, ArgsTy &&... Args) { + unsigned BucketNo = LookupBucketFor(Key); StringMapEntryBase *&Bucket = TheTable[BucketNo]; if (Bucket && Bucket != getTombstoneVal()) return std::make_pair(iterator(TheTable + BucketNo, false), @@ -337,8 +344,7 @@ if (Bucket == getTombstoneVal()) --NumTombstones; - Bucket = - MapEntryTy::Create(KV.first, Allocator, std::move(KV.second)); + Bucket = MapEntryTy::Create(Key, Allocator, std::forward(Args)...); ++NumItems; assert(NumItems + NumTombstones <= NumBuckets); Index: include/llvm/IR/Metadata.h =================================================================== --- include/llvm/IR/Metadata.h +++ include/llvm/IR/Metadata.h @@ -592,7 +592,6 @@ StringMapEntry *Entry; MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {} - MDString(MDString &&) : Metadata(MDStringKind, Uniqued) {} public: static MDString *get(LLVMContext &Context, StringRef Str); Index: lib/IR/Metadata.cpp =================================================================== --- lib/IR/Metadata.cpp +++ lib/IR/Metadata.cpp @@ -397,17 +397,12 @@ MDString *MDString::get(LLVMContext &Context, StringRef Str) { auto &Store = Context.pImpl->MDStringCache; - auto I = Store.find(Str); - if (I != Store.end()) - return &I->second; - - auto *Entry = - StringMapEntry::Create(Str, Store.getAllocator(), MDString()); - bool WasInserted = Store.insert(Entry); - (void)WasInserted; - assert(WasInserted && "Expected entry to be inserted"); - Entry->second.Entry = Entry; - return &Entry->second; + auto I = Store.emplace_second(Str); + auto &MapEntry = I.first->getValue(); + if (!I.second) + return &MapEntry; + MapEntry.Entry = &*I.first; + return &MapEntry; } StringRef MDString::getString() const { Index: unittests/ADT/StringMapTest.cpp =================================================================== --- unittests/ADT/StringMapTest.cpp +++ unittests/ADT/StringMapTest.cpp @@ -358,24 +358,28 @@ namespace { // Simple class that counts how many moves and copy happens when growing a map -struct CountCopyAndMove { +struct CountCtorCopyAndMove { + static unsigned Ctor; static unsigned Move; static unsigned Copy; - CountCopyAndMove() {} + int Data = 0; + CountCtorCopyAndMove(int Data) : Data(Data) { Ctor++; } + CountCtorCopyAndMove() { Ctor++; } - CountCopyAndMove(const CountCopyAndMove &) { Copy++; } - CountCopyAndMove &operator=(const CountCopyAndMove &) { + CountCtorCopyAndMove(const CountCtorCopyAndMove &) { Copy++; } + CountCtorCopyAndMove &operator=(const CountCtorCopyAndMove &) { Copy++; return *this; } - CountCopyAndMove(CountCopyAndMove &&) { Move++; } - CountCopyAndMove &operator=(const CountCopyAndMove &&) { + CountCtorCopyAndMove(CountCtorCopyAndMove &&) { Move++; } + CountCtorCopyAndMove &operator=(const CountCtorCopyAndMove &&) { Move++; return *this; } }; -unsigned CountCopyAndMove::Copy = 0; -unsigned CountCopyAndMove::Move = 0; +unsigned CountCtorCopyAndMove::Copy = 0; +unsigned CountCtorCopyAndMove::Move = 0; +unsigned CountCtorCopyAndMove::Ctor = 0; } // anonymous namespace @@ -385,14 +389,43 @@ // 1 is an "edge value", 32 is an arbitrary power of two, and 67 is an // arbitrary prime, picked without any good reason. for (auto Size : {1, 32, 67}) { - StringMap Map(Size); - CountCopyAndMove::Copy = 0; - CountCopyAndMove::Move = 0; + StringMap Map(Size); + CountCtorCopyAndMove::Move = 0; + CountCtorCopyAndMove::Copy = 0; for (int i = 0; i < Size; ++i) - Map.insert(std::make_pair(Twine(i).str(), CountCopyAndMove())); - EXPECT_EQ((unsigned)Size * 3, CountCopyAndMove::Move); - EXPECT_EQ(0u, CountCopyAndMove::Copy); + Map.insert(std::make_pair(Twine(i).str(), CountCtorCopyAndMove())); + EXPECT_EQ((unsigned)Size * 3, CountCtorCopyAndMove::Move); + EXPECT_EQ(0u, CountCtorCopyAndMove::Copy); } } +TEST(StringMapCustomTest, BracketOperatorCtor) { + StringMap Map; + CountCtorCopyAndMove::Ctor = 0; + Map["abcd"]; + EXPECT_EQ(1u, CountCtorCopyAndMove::Ctor); + // Test that operator[] does not create a value when it is already in the map + CountCtorCopyAndMove::Ctor = 0; + Map["abcd"]; + EXPECT_EQ(0u, CountCtorCopyAndMove::Ctor); +} + +namespace { +struct NonMoveableNonCopyableType { + int Data = 0; + NonMoveableNonCopyableType() = default; + NonMoveableNonCopyableType(int Data) : Data(Data) {} + NonMoveableNonCopyableType(const NonMoveableNonCopyableType &) = delete; + NonMoveableNonCopyableType(NonMoveableNonCopyableType &&) = delete; +}; +} + +// Test that we can "emplace" an element in the map without involving map/move +TEST(StringMapCustomTest, EmplaceTest) { + StringMap Map; + Map.emplace_second("abcd", 42); + EXPECT_EQ(1u, Map.count("abcd")); + EXPECT_EQ(42, Map["abcd"].Data); +} + } // end anonymous namespace