[PATCH] Add standard insert overloads to StringMap

Manuel Klimek klimek at google.com
Mon Jun 16 01:26:01 PDT 2014 

Ok, this is hopefully fixed. I'll monitor closely...


On Mon, Jun 16, 2014 at 10:15 AM, Manuel Klimek <klimek at google.com> wrote:

> On Mon, Jun 16, 2014 at 10:01 AM, Alp Toker <alp at nuanti.com> wrote:
>
>>
>> On 16/06/2014 10:32, David Blaikie wrote:
>>
>>> On Mon, Jun 16, 2014 at 12:28 AM, Alp Toker <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, I'd appreciate it if you would fall back to assuming that people are
> trying to do the right thing instead of insinuating malice.
>
> 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.
> I'm working on a fix. Possibilities are:
> 1. send to sendgrid regardless of email problems, and hope it handles it
> correctly (not sending to invalid mails, but otherwise accepting).
> 2. filter out invalid emails, but send nonetheless
> (I'll probably try to go with (2) first)
>
> Cheers,
> /Manuel
>
>
>>
>> Alp.
>>
>>
>>
>>
>>> - David
>>>
>>>  Alp.
>>>>
>>>>
>>>>> On Sun, Jun 15, 2014 at 6:17 PM, Agustín Bergé <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
>>>>> 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
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140616/59e4e7f1/attachment.html>

More information about the llvm-commits mailing list