[PATCH] Add standard insert overloads to StringMap

[Timur Iskhodzhanov]

I think it wasn't hard to see llvm-commits on CC in Phab.  I can't imagine
a single reason for an ADT review to be private and David responses were
perfectly reasonable for me (note I was one of the reviewers and saw all
the mail).

It looks like this time you've trusted the technology more than you've
trusted people -- if you ask me, you should have much stronger arguments
before blaming people like this.
16 июня 2014 г. 12:07 пользователь "Alp Toker" <alp at nuanti.com> написал:

>
> 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140616/8e06160e/attachment.html>