<p dir="ltr">I'm sorry to see that you've read my email very differently from what I've intended.<br>
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.</p>
<div class="gmail_quote">16 июня 2014 г. 13:00 пользователь "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 11:32, Timur Iskhodzhanov wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
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).<br>
<br>
</blockquote>
<br>
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.<br>
<br>
Did you notice there are two duplicate copies of a large chunk of code in the patch, presumably attempting to provide const-correctness?<br>
<br>
I guess not, but why exclude others from pointing out flaws and improving quality of review?<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
</blockquote>
<br>
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.<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
16 июня 2014 г. 12:07 пользователь "Alp Toker" <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> написал:<br>
<br>
<br>
On 16/06/2014 10:32, David Blaikie wrote:<br>
<br>
On Mon, Jun 16, 2014 at 12:28 AM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br>
<mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
On 16/06/2014 10:01, David Blaikie wrote:<br>
<br>
The test case looks a bit thin - it doesn't test the<br>
return value of<br>
insert, only tests one of the two insert functions the<br>
patch<br>
introduces, and doesn't test positive and negative<br>
cases of insert<br>
(pre-existing element and new insertion)<br>
<br>
Does this function have some/all redundancy with<br>
existing interface?<br>
(should the other insert-like APIs be refactored to<br>
call insert (&<br>
perhaps eventually removed in favor of just using<br>
insert?))<br>
<br>
In the other code review you mention StringMap has an<br>
insert that<br>
returns bool? Yet you're not modifying that version<br>
here? What<br>
arguments does it take that make it distinct from the<br>
two functions<br>
you're adding?<br>
<br>
(though don't get me wrong, this seems like a good<br>
change to make)<br>
<br>
<br>
I'm sorry David, that's not your decision to make. This is<br>
taking the<br>
private review idea way too far.<br>
<br>
The patch should be posted to llvm-commits for public review.<br>
<br>
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<br>
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>
<br>
<br>
<br>
David, could you check your facts instead of firing off defensive<br>
one-liners?<br>
<br>
The revision is private and there is no public record of the patch<br>
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<br>
blaming tools when it gets noticed is so last week.<br>
<br>
Alp.<br>
<br>
<br>
<br>
- David<br>
<br>
Alp.<br>
<br>
<br>
On Sun, Jun 15, 2014 at 6:17 PM, Agustín Bergé<br>
<<a href="mailto:kaballo86@hotmail.com" target="_blank">kaballo86@hotmail.com</a> <mailto:<a href="mailto:kaballo86@hotmail.com" target="_blank">kaballo86@hotmail.com</a>><u></u>><br>
wrote:<br>
<br>
Hi timurrrr, dblaikie, bkramer,<br>
<br>
This patch adds to `StringMap` overloads of<br>
`insert` taking a<br>
`std::pair<K, V>`, following the standard<br>
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<br>
pair into the map if the<br>
key<br>
+ /// isn't already in the map. If the key is<br>
already in the map, it<br>
returns<br>
+ // false and doesn't update the value.<br>
+ std::pair<iterator, bool> insert(const<br>
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<br>
exists in map.<br>
+<br>
+ MapEntryTy *NewItem =<br>
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,<br>
false), true);<br>
+ }<br>
+<br>
+ /// insert - Inserts the specified key/value<br>
pair into the map if the<br>
key<br>
+ /// isn't already in the map. If the key is<br>
already in the map, it<br>
returns<br>
+ // false and doesn't update the value.<br>
+ std::pair<iterator, bool><br>
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<br>
exists in map.<br>
+<br>
+ MapEntryTy *NewItem =<br>
+ MapEntryTy::Create(KV.first, Allocator,<br>
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,<br>
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,<br>
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>
<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> <mailto:<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.<u></u>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>
<br>
<br>
--<br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<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> <mailto:<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.<u></u>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>
<br>
</blockquote>
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
</blockquote></div>