[PATCH] Add standard insert overloads to StringMap
Alp Toker
alp at nuanti.com
Mon Jun 16 03:59:50 PDT 2014
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
More information about the llvm-commits
mailing list