<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Jun 16, 2014 at 10:01 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
On 16/06/2014 10:32, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
On 16/06/2014 10:01, David Blaikie wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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></div>
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.</blockquote><div><br></div><div>Alp, I'd appreciate it if you would fall back to assuming that people are trying to do the right thing instead of insinuating malice.</div>
<div> </div><div>The problem is that somebody registered "Philip Reames <public @ my full name .com>" (which is an invalid email), and now phab eats all emails that it tries to send that include that name. Since people can set up triggers to get notified for incoming reviews, this basically allows anybody to dos-attack phab.</div>
<div>I'm working on a fix. Possibilities are: </div><div>1. send to sendgrid regardless of email problems, and hope it handles it correctly (not sending to invalid mails, but otherwise accepting).</div><div>2. filter out invalid emails, but send nonetheless</div>
<div>(I'll probably try to go with (2) first)</div><div><br></div><div>Cheers,</div><div>/Manuel</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><font color="#888888"><br>
<br>
Alp.</font></span><div class=""><div class="h5"><br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
- David<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Alp.<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style: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>
</div></div></blockquote></div><br></div></div>