[PATCH] Add standard insert overloads to StringMap
Manuel Klimek
klimek at google.com
Mon Jun 16 02:15:11 PDT 2014
On Mon, Jun 16, 2014 at 11:00 AM, Alp Toker <alp at nuanti.com> wrote:
>
> On 16/06/2014 11:32, Timur Iskhodzhanov wrote:
>
>>
>> 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).
>>
>>
> That attitude is part of the problem. I don't think it's OK for the patch
> to skip the mailing list Just because it seemed reasonable to you and David.
>
I'm not sure what you're getting at. My understanding is that everybody
expected the mail to go to the list. It didn't go to the list mainly due to
a technical problem, but that wasn't noticed until you pointed it out.
> Did you notice there are two duplicate copies of a large chunk of code in
> the patch, presumably attempting to provide const-correctness?
>
> I guess not, but why exclude others from pointing out flaws and improving
> quality of review?
So, if I understand this correctly, you believe that it was the intention
of somebody for this to not go to the list?
> 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.
>>
>>
> It turns out the technology was right -- as Manuel pointed out, the patch
> was *never sent to the list* and only you guys had the chance to comment.
>
The technology was wrong - it didn't send the mail to the list because of
an error.
>
> Alp.
>
>
> 16 июня 2014 г. 12:07 пользователь "Alp Toker" <alp at nuanti.com <mailto:
>> 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
>> <mailto: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 <mailto: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 <mailto: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 <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
> --
> 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/5aff11bf/attachment.html>
More information about the llvm-commits
mailing list