[PATCH] Add standard insert overloads to StringMap

[David Blaikie]

>>! In D4153#5, @K-ballo wrote:
>>>! In D4153#4, @dblaikie wrote:
>> The test case looks a bit thin - it doesn't test the return value of
>> insert, only tests one of the two insert functions the patch
>> introduces, and doesn't test positive and negative cases of insert
>> (pre-existing element and new insertion)
> 
> I will add those.
> 
>> Does this function have some/all redundancy with existing interface?
>> (should the other insert-like APIs be refactored to call insert (&
>> perhaps eventually removed in favor of just using insert?))
> 
> The new overloads behave exactly like `GetOrCreateValue`, except that the parameters and return types are different. I'll reimplement it in terms of `insert`.

Great, thanks!

>> 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).

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

http://reviews.llvm.org/D4153