[PATCH] Add standard insert overloads to StringMap

[David Blaikie]

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)

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

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?

(though don't get me wrong, this seems like a good change to make)


On Sun, Jun 15, 2014 at 6:17 PM, Agustín Bergé <kaballo86 at hotmail.com> wrote:
> Hi timurrrr, dblaikie, bkramer,
>
> This patch adds to `StringMap` overloads of `insert` taking a `std::pair<K, V>`, following the standard associative container interface. This was suggested in http://reviews.llvm.org/D4130
>
> http://reviews.llvm.org/D4153
>
> Files:
>   include/llvm/ADT/StringMap.h
>   unittests/ADT/StringMapTest.cpp
>
> Index: include/llvm/ADT/StringMap.h
> ===================================================================
> --- include/llvm/ADT/StringMap.h
> +++ include/llvm/ADT/StringMap.h
> @@ -323,6 +323,51 @@
>      return true;
>    }
>
> +  /// insert - Inserts the specified key/value pair into the map if the key
> +  /// isn't already in the map. If the key is already in the map, it returns
> +  // false and doesn't update the value.
> +  std::pair<iterator, bool> insert(const std::pair<StringRef, ValueTy> &KV) {
> +    unsigned BucketNo = LookupBucketFor(KV.first);
> +    StringMapEntryBase *&Bucket = TheTable[BucketNo];
> +    if (Bucket && Bucket != getTombstoneVal())
> +      return std::make_pair(iterator(&Bucket, false),
> +                            false); // Already exists in map.
> +
> +    MapEntryTy *NewItem = MapEntryTy::Create(KV.first, Allocator, KV.second);
> +
> +    if (Bucket == getTombstoneVal())
> +      --NumTombstones;
> +    Bucket = NewItem;
> +    ++NumItems;
> +    assert(NumItems + NumTombstones <= NumBuckets);
> +
> +    RehashTable();
> +    return std::make_pair(iterator(&Bucket, false), true);
> +  }
> +
> +  /// insert - Inserts the specified key/value pair into the map if the key
> +  /// isn't already in the map. If the key is already in the map, it returns
> +  // false and doesn't update the value.
> +  std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> &&KV) {
> +    unsigned BucketNo = LookupBucketFor(KV.first);
> +    StringMapEntryBase *&Bucket = TheTable[BucketNo];
> +    if (Bucket && Bucket != getTombstoneVal())
> +      return std::make_pair(iterator(&Bucket, false),
> +                            false); // Already exists in map.
> +
> +    MapEntryTy *NewItem =
> +        MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));
> +
> +    if (Bucket == getTombstoneVal())
> +      --NumTombstones;
> +    Bucket = NewItem;
> +    ++NumItems;
> +    assert(NumItems + NumTombstones <= NumBuckets);
> +
> +    RehashTable();
> +    return std::make_pair(iterator(&Bucket, false), true);
> +  }
> +
>    // clear - Empties out the StringMap
>    void clear() {
>      if (empty()) return;
> Index: unittests/ADT/StringMapTest.cpp
> ===================================================================
> --- unittests/ADT/StringMapTest.cpp
> +++ unittests/ADT/StringMapTest.cpp
> @@ -203,6 +203,13 @@
>    assertSingleItemMap();
>  }
>
> +// Test insert(pair<K, V>) method
> +TEST_F(StringMapTest, InsertTestPair) {
> +  testMap.insert(std::make_pair(testKeyFirst, testValue));
> +  EXPECT_EQ(1u, testMap.size());
> +  EXPECT_EQ(testValue, testMap[testKeyFirst]);
> +}
> +
>  // Create a non-default constructable value
>  struct StringMapTestStruct {
>    StringMapTestStruct(int i) : i(i) {}