[PATCH] Add standard insert overloads to StringMap
Manuel Klimek
klimek at google.com
Mon Jun 16 01:15:39 PDT 2014
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/5ee94a54/attachment.html>
More information about the llvm-commits
mailing list