[PATCH] Add standard insert overloads to StringMap

[Alp Toker]

On 16/06/2014 10:32, David Blaikie wrote:
> 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, could you check your facts instead of firing off defensive 
one-liners?

The revision is private and there is no public record of the patch at all:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140616/thread.html

Sneaking commits into SVN without inviting public review, and blaming 
tools when it gets noticed is so last week.

Alp.


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

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