StringMap::operator[] should not default construct the value when the key is in the map

[mehdi_amini]

Remove unrelated changes

[llvm-commits]

Ah, this is why. LGTM.

[mehdi_amini]

(mark as accepted, per Duncan)

[mehdi_amini]

Has been unfortunately squashed in r264386

include/llvm/ADT/StringMap.h

public:
   /// constructed value if no such entry exists.
   ValueTy lookup(StringRef Key) const {
     const_iterator it = find(Key);
     if (it != end())
       return it->second;
     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.
   size_type count(StringRef Key) const {
     return find(Key) == end() ? 0 : 1;
   }
 
   /// insert - Insert the specified key/value pair into the map.  If the key

include/llvm/IR/Metadata.h

class MDString : public Metadata {
   friend class StringMapEntry<MDString>;
 
   MDString(const MDString &) = delete;
   MDString &operator=(MDString &&) = delete;
   MDString &operator=(const MDString &) = delete;
 
   StringMapEntry<MDString> *Entry;
   MDString() : Metadata(MDStringKind, Uniqued), Entry(nullptr) {}
+  MDString(MDString &&R) = delete;
 
 public:
   static MDString *get(LLVMContext &Context, StringRef Str);
   static MDString *get(LLVMContext &Context, const char *Str) {
     return get(Context, Str ? StringRef(Str) : StringRef());
   }
 
   StringRef getString() const;

unittests/ADT/StringMapTest.cpp

TEST_F(StringMapTest, MoveDtor) {
 
   B = StringMap<Countable>();
   ASSERT_EQ(InstanceCount, 0);
   ASSERT_TRUE(B.empty());
 }
 
 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;
   int Data = 0;
-  CountCopyAndMove() {}
-  CountCopyAndMove(unsigned int Data) : Data(Data) {}
+  CountCtorCopyAndMove(unsigned 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
 
 // Make sure creating the map with an initial size of N actually gives us enough
 // buckets to insert N items without increasing allocation size.
 TEST(StringMapCustomTest, InitialSizeTest) {
   for (unsigned Size = 1; Size <= 1024; ++Size) {
-    StringMap<CountCopyAndMove> Map(Size);
-    CountCopyAndMove::Copy = 0;
-    CountCopyAndMove::Move = 0;
+    StringMap<CountCtorCopyAndMove> Map(Size);
+    CountCtorCopyAndMove::Copy = 0;
+    CountCtorCopyAndMove::Move = 0;
     for (unsigned i = 0; i < Size; ++i)
-      Map.insert(std::make_pair(Twine(i).str(), CountCopyAndMove()));
-    EXPECT_EQ(Size * 2, CountCopyAndMove::Move);
-    EXPECT_EQ((unsigned)0, CountCopyAndMove::Copy);
+      Map.insert(std::make_pair(Twine(i).str(), CountCtorCopyAndMove()));
+    EXPECT_EQ(Size * 2, CountCtorCopyAndMove::Move);
+    EXPECT_EQ((unsigned)0, CountCtorCopyAndMove::Copy);
   }
 }
 
 // Test that we can "emplace" an element in the map without involving map/move
 TEST(StringMapCustomTest, EmplaceTest) {
-  StringMap<CountCopyAndMove> Map(1);
-  CountCopyAndMove::Copy = 0;
-  CountCopyAndMove::Move = 0;
+  StringMap<CountCtorCopyAndMove> Map(1);
+  CountCtorCopyAndMove::Copy = 0;
+  CountCtorCopyAndMove::Move = 0;
   Map.emplace_second("abcd", 42);
   EXPECT_EQ(1u, Map.count("abcd"));
-  EXPECT_EQ(0u, CountCopyAndMove::Copy);
-  EXPECT_EQ(0u, CountCopyAndMove::Move);
+  EXPECT_EQ(0u, CountCtorCopyAndMove::Copy);
+  EXPECT_EQ(0u, CountCtorCopyAndMove::Move);
+  CountCtorCopyAndMove::Ctor = 0;
   EXPECT_EQ(42, Map["abcd"].Data);
+  EXPECT_EQ(0u, CountCtorCopyAndMove::Copy);
+  EXPECT_EQ(0u, CountCtorCopyAndMove::Move);
+}
+
+TEST(StringMapCustomTest, BracketOperatorCtor) {
+  StringMap<CountCtorCopyAndMove> 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);
 }
 
 } // end anonymous namespace