[PATCH] Add standard insert overloads to StringMap
Timur Iskhodzhanov
timurrrr at google.com
Mon Jun 16 04:02:24 PDT 2014
I by no means blame you for pointing that out. I don't support the exact
way it was done though.
16 июня 2014 г. 15:00 пользователь "Alp Toker" <alp at nuanti.com> написал:
>
> On 16/06/2014 13:28, Timur Iskhodzhanov wrote:
>
>>
>> Some of these are good questions for whoever runs Phab (not David AFAIK),
>> but for a different discussion (feel free to start a new thread on the
>> list).
>>
>> I see nothing wrong David had done intentionally during this review, so
>> I'd prefer if you apologize to him for the rant and we get back to work
>> (and I'll get back to my sick leave).
>>
>>
> Timur, that's out of order. The developer policy makes it clear the list
> is the place for patches. It's also made clear:
>
> " If someone sends you a patch privately, encourage them to submit it to
> the appropriate list first."
>
> http://llvm.org/docs/DeveloperPolicy.html
>
> It's basically not OK that every time the community is excluded from
> review, some tool gets blamed and the person pointing it out gets
> personally attacked.
>
> The policy makes it clear that responsibility lies with the author. If I
> hadn't pointed this out, the code would have just slipped through as
> happens quite often, without anyone outside the closed circle getting a say.
>
> How about we focus on fixing that instead of making this about
> personalities?
>
> The patch workflow is becoming increasingly dysfunctional with 3.5 just
> round the corner and I'm concerned, and speaking up about it.
>
> Alp.
>
>
>
> 16 июня 2014 г. 14:01 пользователь "Alp Toker" <alp at nuanti.com <mailto:
>> alp at nuanti.com>> написал:
>>
>>
>> On 16/06/2014 12:11, Timur Iskhodzhanov wrote:
>>
>>
>> I'm sorry to see that you've read my email very differently
>> from what I've intended.
>> I think you should calm down a little bit and read the Phab
>> log in our shoes. I don't think anyone on this thread tried
>> to do anything suspicious on purpose.
>>
>>
>> Okay, let's try to understand how the patch workflow goes in your
>> shoes. I think more transparency about the web-based review
>> workflow is needed.
>>
>> None of the below is actually clear to people using the mailing
>> list for review, and very often the web-based changes seem fly by
>> in read-only form or on-list reviews get summarily ignored:
>>
>> How are the CC lists for patches decided? The names don't seem to
>> be derived from the canonical CODE_OWNERS.txt -- are they
>> maintained in SVN or a separate repository?
>>
>> Is there public access to see who is assigned to what piece of
>> code? How are these decided, and are mailing list reviewers able
>> to join the CC list or is it exclusive to web-based reviewers?
>>
>> What determines if a patch is in "private pre-review" and excluded
>> from public mailing lists? Is there some kind of subscription-only
>> mailing list to track such patches, or are they kept in a private
>> database?
>>
>> Why do arbitrary names show up on the "Reviewers" line when review
>> was actually been done by someone on-list?
>>
>> Why do on-list reviews of web-based patches frequently get
>> ignored? Are the patch authors ignoring something, or is it a bug
>> in the web interface?
>>
>> Alp.
>>
>>
>>
>> 16 июня 2014 г. 13:00 пользователь "Alp Toker" <alp at nuanti.com
>> <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>> <mailto:alp at nuanti.com>>> написал:
>>
>>
>> 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.
>>
>> 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?
>>
>> 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.
>>
>> Alp.
>>
>>
>> 16 июня 2014 г. 12:07 пользователь "Alp Toker"
>> <alp at nuanti.com <mailto:alp at nuanti.com>
>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>
>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>
>> <mailto: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>
>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>>
>> <mailto:alp at nuanti.com <mailto:alp at nuanti.com>
>> <mailto: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>
>> <mailto:kaballo86 at hotmail.com
>> <mailto:kaballo86 at hotmail.com>> <mailto:kaballo86 at hotmail.com
>> <mailto:kaballo86 at hotmail.com>
>> <mailto: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>
>> <mailto:llvm-commits at cs.uiuc.edu
>> <mailto:llvm-commits at cs.uiuc.edu>>
>> <mailto:llvm-commits at cs.uiuc.edu
>> <mailto:llvm-commits at cs.uiuc.edu>
>> <mailto: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>
>> <mailto:llvm-commits at cs.uiuc.edu
>> <mailto:llvm-commits at cs.uiuc.edu>>
>> <mailto:llvm-commits at cs.uiuc.edu
>> <mailto:llvm-commits at cs.uiuc.edu>
>> <mailto: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
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140616/2711b8a9/attachment.html>
More information about the llvm-commits
mailing list