[PATCH] Add standard insert overloads to StringMap

[David Blaikie]

Sounds like we're on the right track - last changes are minor (just to remove the hash value check). Please commit once that's done.

================
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:
> > 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.
> Here is what happens: There is only one bucket in the map, so the insert naturally places the (pointer to the) new element there. Since the map is full, more buckets are added and elements are rehashed. For this particular input, the rehash causes the (pointer to the) new element to be moved to the second bucket. That's the condition that I would like to test for.
> 
> I am fine with your suggestion, as I don't have a better way to address this. I'll work on those changes.
OK, so the bit I was missing was that you need a specific hash function to cause the element to move buckets... yeah, that's tricky.

I don't mind not checking the hash function still. Yes, if someone ripped that code out and didn't update the bucket during insertion the test might still pass. Pity, not sure there's much to be done about it.

http://reviews.llvm.org/D4153