[PATCH] Add standard insert overloads to StringMap

[Agustín Bergé]

>>> In the other code review you mention StringMap has an insert that
>>> returns bool? Yet you're not modifying that version here? What
>>> arguments does it take that make it distinct from the two functions
>>> you're adding?
>> 
>> The existing `insert` is defined as `bool insert(MapEntryTy *KeyValue)`, it takes a pointer to an already created `MapEntryTy` and acquires ownership of it. AFAIK, this is the only piece of functionality that would not be covered by a standard associative sequence interface.
> 
> Do any of the callers (at a glance) do anything fancy with this? Do they get a pre-allocated MapEntryTy object from somewhere far flung (from another map, from this map under another key, etc) - or do they just allocate it and pass it in immediately?
> 
> If it doesn't seem to add any value, it'd be nice to remove it (eventually - that task doesn't have to be yours to bear, but if it looks like that's the right path then a FIXME comment suggesting that direction could be useful).

I tried commenting out that overload, and I can immediately see `ValueSymbolTable::reinsertValue` (llvm/lib/IR/ValueSymbolTable.cpp:37) doing a reinsertion of an already allocated entry. It doesn't seem like removing it would be an option, given the special nature of `StringMap<>::MapEntryTy`.

================
Comment at: include/llvm/ADT/StringMap.h:359
@@ +358,3 @@
+    MapEntryTy *NewItem =
+        MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));
+
----------------
David Blaikie wrote:
> It looks like the only difference between these functions is the "std::move" here - if we can guarantee reasonably cheaply-movable value types, we could replace the two overloads with one function that takes the std::pair by value, and std::moves the value unconditionally.
> 
> If it seems like there are some types that will never be cheaply movable, then keeping the two overloads might be necessary - but the common suffix and prefix could be factored into another function (or the whole function could be refactored into one common utility with a lambda to choose how to process the value... ). Considering how to handle the deduplication across other operations could help point to the right factoring.
I noticed that `GetOrCreateValue` is already taking arguments by value and moving unconditionally, so I will be taking that approach for the next iteration of the patch.

http://reviews.llvm.org/D4153