[PATCH] Add standard insert overloads to StringMap
Aaron Ballman
aaron at aaronballman.com
Mon Jun 16 06:50:10 PDT 2014
Okay, so *now* the original email just came in for me. So perhaps Phab
was just running very (12 hours) behind on this?
~Aaron
On Mon, Jun 16, 2014 at 7:56 AM, Manuel Klimek <klimek at google.com> wrote:
> 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
>>>>
>>>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list