[PATCH] Add standard insert overloads to StringMap
Alp Toker
alp at nuanti.com
Mon Jun 16 03:01:42 PDT 2014
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>> написал:
>
>
> 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>>> написал:
>
>
> 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>>> 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>>>
> 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>>
> 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>>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
> --
> http://www.nuanti.com
> the browser experts
>
--
http://www.nuanti.com
the browser experts
More information about the llvm-commits
mailing list