<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 16, 2014 at 11:00 AM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class=""><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></div>
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></blockquote><div><br></div><div>I'm not sure what you're getting at. My understanding is that everybody expected the mail to go to the list. It didn't go to the list mainly due to a technical problem, but that wasn't noticed until you pointed it out.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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?</blockquote><div><br></div><div>So, if I understand this correctly, you believe that it was the intention of somebody for this to not go to the list?</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<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></div>
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></blockquote><div><br></div><div>The technology was wrong - it didn't send the mail to the list because of an error.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<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>>> написал:<div class="">
<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></div><div><div class="h5">
<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></div></div>
<<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>><div><div class="h5">
<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></div></div>
<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>><div class="">
<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></div>
<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><div class="HOEnZb"><div class="h5">
<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>
</div></div></blockquote></div><br></div></div>