[PATCH] Add standard insert overloads to StringMap
Manuel Klimek
klimek at google.com
Mon Jun 16 07:06:42 PDT 2014
On Mon, Jun 16, 2014 at 3:50 PM, Aaron Ballman <aaron at aaronballman.com>
wrote:
> Okay, so *now* the original email just came in for me. So perhaps Phab
> was just running very (12 hours) behind on this?
>
It was a retry - I just fixed a problem where phab got dos-attacked by a
user registering an invalid email, and then subscribing to basically all
initial patches, and phab started to retry the initial messages it wasn't
able to send out before.
>
> ~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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140616/eb91cb98/attachment.html>
More information about the llvm-commits
mailing list