[PATCH] Add standard insert overloads to StringMap

[David Blaikie]

================
Comment at: unittests/ADT/StringMapTest.cpp:233
@@ +232,3 @@
+  EXPECT_EQ(1, t.getNumBuckets());
+  EXPECT_EQ(3916023477, HashString("abcdef"));
+
----------------
Agustín Bergé wrote:
> David Blaikie wrote:
> > What is this EXPECT test for?
> As I said before, this test is not correct. In order for the test case to be useful, it has to have knowledge of the hashing function and the internals of `StringMap`. If either of those change, this test might became meaningless. That EXPECT was my attempt to have this test fail if the hashing function changes, but is of course wrong. I really don't know how to reliably reproduce a situation that depends on several implementation details.
What change to the hash value fo the string could cause the test to become invalid, and in what way would the test be invalid?

Is the concern that the hashed value of the string might fit in the first bucket that's already there? (why doesn't it anyway? is that one bucket a sentinel? If it's a sentinel then it doesn't matter what the hashed value of the string is - it'll always cause a rehashing/growth)

In either case, the EXPECT num buckets == 1, insert, then EXPECT num buckets == 2 seems to suffice to verify that a rehash/growth has occurred, does it not?

I'm actually OK with the test just doing that (dropping the hash value checking, keeping the "number of buckets changed" check) and putting a big comment explaining how brittle this test is.

http://reviews.llvm.org/D4153