[PATCH] Add standard insert overloads to StringMap
Manuel Klimek
klimek at google.com
Mon Jun 16 04:44:44 PDT 2014
On Mon, Jun 16, 2014 at 1:34 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> I had seen messages from the mailing list bot telling me as a revision
> owner that the automated e-mail from Phab needs approval since it addressed
> too many e-mails. This may be another possible point of failure. Otherwise,
> I find Phab to be much more orderly way of reviewing than the list.
>
> Right now the default in Phab is no CC to the lists. How about making the
> default CC to llvm-commits and/or cfe-commits?
>
I think using both would be too spammy, and deciding on one would need
somebody to put some implementation effort into it. I think engineers are
generally very capable of adding the right list to CC, much like they are
when sending emails.
Cheers,
/Manuel
>
>
>
> 2014-06-16 14:02 GMT+03:00 Timur Iskhodzhanov <timurrrr at google.com>:
>
>> 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
>>>
>>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
> _______________________________________________
> 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/61b1547e/attachment.html>
More information about the llvm-commits
mailing list