[PATCH] Add standard insert overloads to StringMap

[David Blaikie]

On Mon, Jun 16, 2014 at 12:28 AM, Alp Toker <alp at nuanti.com> wrote:
>
> 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.

I'm not sure what you're referring to, but I guess it's a Phab
bug/mail delivery issue, because I replied to the on-list mail with an
on-list reply. This is a code review occurring on llvm-commits.

Perhaps it ended up in your spam filter?

- David

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