<p dir="ltr">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).</p>
<p dir="ltr">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.</p>
<div class="gmail_quote">16 июня 2014 г. 12:07 пользователь "Alp Toker" <<a href="mailto:alp@nuanti.com">alp@nuanti.com</a>> написал:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 16/06/2014 10:32, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On Mon, Jun 16, 2014 at 12:28 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
On 16/06/2014 10:01, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The test case looks a bit thin - it doesn't test the return value of<br>
insert, only tests one of the two insert functions the patch<br>
introduces, and doesn't test positive and negative cases of insert<br>
(pre-existing element and new insertion)<br>
<br>
Does this function have some/all redundancy with existing interface?<br>
(should the other insert-like APIs be refactored to call insert (&<br>
perhaps eventually removed in favor of just using insert?))<br>
<br>
In the other code review you mention StringMap has an insert that<br>
returns bool? Yet you're not modifying that version here? What<br>
arguments does it take that make it distinct from the two functions<br>
you're adding?<br>
<br>
(though don't get me wrong, this seems like a good change to make)<br>
</blockquote>
<br>
I'm sorry David, that's not your decision to make. This is taking the<br>
private review idea way too far.<br>
<br>
The patch should be posted to llvm-commits for public review.<br>
</blockquote>
I'm not sure what you're referring to, but I guess it's a Phab<br>
bug/mail delivery issue, because I replied to the on-list mail with an<br>
on-list reply. This is a code review occurring on llvm-commits.<br>
<br>
Perhaps it ended up in your spam filter?<br>
</blockquote>
<br>
<br>
David, could you check your facts instead of firing off defensive one-liners?<br>
<br>
The revision is private and there is no public record of the patch at all:<br>
<br>
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140616/thread.html" target="_blank">http://lists.cs.uiuc.edu/<u></u>pipermail/llvm-commits/Week-<u></u>of-Mon-20140616/thread.html</a><br>
<br>
Sneaking commits into SVN without inviting public review, and blaming tools when it gets noticed is so last week.<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
- David<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Alp.<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Sun, Jun 15, 2014 at 6:17 PM, Agustín Bergé <<a href="mailto:kaballo86@hotmail.com" target="_blank">kaballo86@hotmail.com</a>><br>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi timurrrr, dblaikie, bkramer,<br>
<br>
This patch adds to `StringMap` overloads of `insert` taking a<br>
`std::pair<K, V>`, following the standard associative container interface.<br>
This was suggested in <a href="http://reviews.llvm.org/D4130" target="_blank">http://reviews.llvm.org/D4130</a><br>
<br>
<a href="http://reviews.llvm.org/D4153" target="_blank">http://reviews.llvm.org/D4153</a><br>
<br>
Files:<br>
include/llvm/ADT/StringMap.h<br>
unittests/ADT/StringMapTest.<u></u>cpp<br>
<br>
Index: include/llvm/ADT/StringMap.h<br>
==============================<u></u>==============================<u></u>=======<br>
--- include/llvm/ADT/StringMap.h<br>
+++ include/llvm/ADT/StringMap.h<br>
@@ -323,6 +323,51 @@<br>
return true;<br>
}<br>
<br>
+ /// insert - Inserts the specified key/value pair into the map if the<br>
key<br>
+ /// isn't already in the map. If the key is already in the map, it<br>
returns<br>
+ // false and doesn't update the value.<br>
+ std::pair<iterator, bool> insert(const std::pair<StringRef, ValueTy><br>
&KV) {<br>
+ unsigned BucketNo = LookupBucketFor(KV.first);<br>
+ StringMapEntryBase *&Bucket = TheTable[BucketNo];<br>
+ if (Bucket && Bucket != getTombstoneVal())<br>
+ return std::make_pair(iterator(&<u></u>Bucket, false),<br>
+ false); // Already exists in map.<br>
+<br>
+ MapEntryTy *NewItem = MapEntryTy::Create(KV.first, Allocator,<br>
KV.second);<br>
+<br>
+ if (Bucket == getTombstoneVal())<br>
+ --NumTombstones;<br>
+ Bucket = NewItem;<br>
+ ++NumItems;<br>
+ assert(NumItems + NumTombstones <= NumBuckets);<br>
+<br>
+ RehashTable();<br>
+ return std::make_pair(iterator(&<u></u>Bucket, false), true);<br>
+ }<br>
+<br>
+ /// insert - Inserts the specified key/value pair into the map if the<br>
key<br>
+ /// isn't already in the map. If the key is already in the map, it<br>
returns<br>
+ // false and doesn't update the value.<br>
+ std::pair<iterator, bool> insert(std::pair<StringRef, ValueTy> &&KV) {<br>
+ unsigned BucketNo = LookupBucketFor(KV.first);<br>
+ StringMapEntryBase *&Bucket = TheTable[BucketNo];<br>
+ if (Bucket && Bucket != getTombstoneVal())<br>
+ return std::make_pair(iterator(&<u></u>Bucket, false),<br>
+ false); // Already exists in map.<br>
+<br>
+ MapEntryTy *NewItem =<br>
+ MapEntryTy::Create(KV.first, Allocator, std::move(KV.second));<br>
+<br>
+ if (Bucket == getTombstoneVal())<br>
+ --NumTombstones;<br>
+ Bucket = NewItem;<br>
+ ++NumItems;<br>
+ assert(NumItems + NumTombstones <= NumBuckets);<br>
+<br>
+ RehashTable();<br>
+ return std::make_pair(iterator(&<u></u>Bucket, false), true);<br>
+ }<br>
+<br>
// clear - Empties out the StringMap<br>
void clear() {<br>
if (empty()) return;<br>
Index: unittests/ADT/StringMapTest.<u></u>cpp<br>
==============================<u></u>==============================<u></u>=======<br>
--- unittests/ADT/StringMapTest.<u></u>cpp<br>
+++ unittests/ADT/StringMapTest.<u></u>cpp<br>
@@ -203,6 +203,13 @@<br>
assertSingleItemMap();<br>
}<br>
<br>
+// Test insert(pair<K, V>) method<br>
+TEST_F(StringMapTest, InsertTestPair) {<br>
+ testMap.insert(std::make_pair(<u></u>testKeyFirst, testValue));<br>
+ EXPECT_EQ(1u, testMap.size());<br>
+ EXPECT_EQ(testValue, testMap[testKeyFirst]);<br>
+}<br>
+<br>
// Create a non-default constructable value<br>
struct StringMapTestStruct {<br>
StringMapTestStruct(int i) : i(i) {}<br>
</blockquote>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</blockquote></blockquote>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
______________________________<u></u>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/llvm-commits</a><br>
</blockquote></div>