[PATCH] Add standard insert overloads to StringMap

[Alp Toker]

On 16/06/2014 10:01, David Blaikie 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)
>
> 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)

I'm sorry David, that's not your decision to make. This is taking the 
private review idea way too far.

The patch should be posted to llvm-commits for public review.

Alp.

>
>
> 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) {}
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-- 
http://www.nuanti.com
the browser experts