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