[PATCH] Add standard insert overloads to StringMap

[David Blaikie]

>>! In D4153#8, @K-ballo wrote:
> Changes in this iteration:
> 
> - Simplified implementation.
> - Corrected doxygen comment.
> - Reimplemented `GetOrCreateValue` on top of the new `insert`.
> - Improved test case.
> 
> By having `GetOrCreateValue` call the new `insert` overload internally, I was able to detect a gross oversight on my first implementation. Inserting an element in the map can trigger a table rehashing, in which case the returned iterator was pointing to the wrong bucket. I sorted this in the simplest way that I could think that would give maximum performance, and that is by modifying `RehashTable` to map an old bucket number into the corresponding new bucket number. 

Seems like a reasonable fix.

> 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?

================
Comment at: include/llvm/ADT/StringMap.h:338
@@ +337,3 @@
+
+    MapEntryTy *NewItem =
+        MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));
----------------
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?)

================
Comment at: lib/Support/StringMap.cpp:214
@@ -213,3 +211,3 @@
       unsigned FullHash = HashTable[I];
-      unsigned NewBucket = FullHash & (NewSize-1);
       if (!NewTableArray[NewBucket]) {
----------------
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.


================
Comment at: unittests/ADT/StringMapTest.cpp:215
@@ +214,3 @@
+  EXPECT_EQ(testValue, testMap[testKeyFirst]);
+  EXPECT_EQ(NewIt->first(), testKeyFirst);
+  EXPECT_EQ(NewIt->second, testValue);
----------------
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.

================
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());
----------------
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.

http://reviews.llvm.org/D4153