[PATCH] Add standard insert overloads to StringMap

[Agustín Bergé]

>> I would like to add a test case that triggers a table rehash but I'm not sure how to do that, suggestions?
> 
> Not sure, actually. It looks like the growth function is the usual sort of thing, so probably just inserting a second element should cause a rehash - no?

I did not see this in my previous tests using up to 10 values, because the hash table starts with 16 buckets. I see that I can specify how many buckets to use on construction, so creating a map with only 1 bucket and adding two elements to it would trigger a rehash. Then I'd have to make sure that the rehash actually moves the bucket around, which is the tricky part.

================
Comment at: include/llvm/ADT/StringMap.h:338
@@ +337,3 @@
+
+    MapEntryTy *NewItem =
+        MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));
----------------
David Blaikie wrote:
> Do you need the "NewItem" variable? Could you just assign straight to Bucket?
> 
> (you could do the Bucket == getTombstoneVal() before the MapEntryTy::Create? Or does Create need to worry about the old tombstone count?)
I followed the implementation of `GetOrCreateValue` here, but I don't see any obvious reason why I couldn't just do the assignment directly.

================
Comment at: lib/Support/StringMap.cpp:214
@@ -213,3 +211,3 @@
       unsigned FullHash = HashTable[I];
-      unsigned NewBucket = FullHash & (NewSize-1);
       if (!NewTableArray[NewBucket]) {
----------------
David Blaikie wrote:
> Please remove formatting changes to lines unrelated to your change.
> 
> If you're using clang-format to reformat, you can use the git/diff clang format scripts to just reformat the lines your code changes.
> 
Thanks for that tip! I was wondering how people were able to use clang-format in situations like these.

================
Comment at: unittests/ADT/StringMapTest.cpp:215
@@ +214,3 @@
+  EXPECT_EQ(testValue, testMap[testKeyFirst]);
+  EXPECT_EQ(NewIt->first(), testKeyFirst);
+  EXPECT_EQ(NewIt->second, testValue);
----------------
David Blaikie wrote:
> That's an annoyingly creepy difference (first being a function, second being a value) - probably be good if first was a StringRef member to begin with. Again, not your problem though.
I had initially added the `insert` overload doing batch insertion, which is an interesting one to have as it could avoid extra table rehashing. I did not include it in this patch because of the weirdness of `StringMapIterator`, which isn't even an _Iterator_.

This would be a big breaking change, but if that is something acceptable I could look into making `StringMap` more like an `std::map`, or at least more like `DenseMap`. This would be of course not part of this patch.

================
Comment at: unittests/ADT/StringMapTest.cpp:221
@@ +220,3 @@
+  std::tie(ExistingIt, Inserted) =
+      testMap.insert(std::make_pair(testKeyFirst, testValue + 1));
+  EXPECT_EQ(1u, testMap.size());
----------------
David Blaikie wrote:
> While it's true this (testValue  + 1) is an xvalue, it's not really an interesting xvalue - a test case with a move-only value type might be more interesting and fully exercise the std::move stuff in insert.
My intention here is actually to test that the value is not updated when the pair already exists in the map. Since `insert` is taking the argument by value, I did not considered adding a special test case for moves. In order for such test to be meaningful, it would have to keep track of the number of copies, and `StringMap` doesn't fully work with movable-only types (`StringMapEntry::set_value` makes copies).

http://reviews.llvm.org/D4153