[PATCH] Add standard insert overloads to StringMap
Manuel Klimek
klimek at google.com
Mon Jun 16 04:56:12 PDT 2014
On Mon, Jun 16, 2014 at 1:50 PM, Yaron Keren <yaron.keren at gmail.com> wrote:
> Hi Manuel,
>
> The situation with Phab is somewhat different than the e-mail lists since
> you can create a revision without CC the lists but you can't post on the
> lists without mailing it...
>
You can write emails to people personally without cc'ing the list.
I can (and have) sent patches to people off-list, when I just wanted to
throw around an idea that I didn't think was ready for review, or somebody
else wanted to patch it in. Of course that doesn't count as review...
> Sending to both lists of course make no sense, I thought about having a CC
> default of the two lists and the engineer will choose which one (or
> possibly both) to remove while creating the revision rather having none and
> the engineer choosing which one to add. This way one can't forget to add
> the CC list since they are the default.
>
I still believe that this can be easily addressed with education (but I am
aware that I might be wrong), the same way I point out to somebody who
sends me reviews privately via email (yes, that happens, too) to send it to
cfe-commits.
>
> Yaron
>
>
>
>
> 2014-06-16 14:44 GMT+03:00 Manuel Klimek <klimek at google.com>:
>
> 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/7b67c9bb/attachment.html>
More information about the llvm-commits
mailing list