Bug 134857 – Add an ensure function on HashMap that takes a key and a function to make the lazy value initialization idiom easier

• Component: New Bugs
• Status: RESOLVED FIXED

[Sam Weinig]

Add an overload of HashMap::add that takes a function instead of the value to make the lazy value initialization idiom easier

[Sam Weinig]

Created attachment 234806 [details]
Patch

[Sam Weinig]

Created attachment 234808 [details]
Patch

[WebKit Commit Bot]

Attachment 234808 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:158:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 234809 [details]
Patch

[WebKit Commit Bot]

Attachment 234809 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:158:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 234810 [details]
Patch

[WebKit Commit Bot]

Attachment 234810 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:158:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 234813 [details]
Patch

[WebKit Commit Bot]

Attachment 234813 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/HashMap.h:353:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/HashMap.h:360:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:158:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:162:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 271520 [details]
Patch

[WebKit Commit Bot]

Attachment 271520 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:515:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:519:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:530:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:533:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:543:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:547:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:561:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:564:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 8 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

I decided to change my approach and instead of overloading add, which is complicated and error prone, I added a new function called "ensure". This greatly simplifies the patch.

[Alex Christensen]

Comment on attachment 271520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271520&action=review

Add a test that asserts that the functor is not called if not needed.

TEST(WTF_HashMap, Ensure_Functor)
{
    HashMap<unsigned, unsigned> map;
    {
        bool functorCalled = false;
        unsigned value = 0;
        auto functor = [&] { functorCalled = true; return ++value; };
        
        map.add(1, 0);
        map.ensure(1, functor);
        EXPECT_FALSE(functorCalled);
        EXPECT_EQ(map.get(1), 0);
        
        map.remove(1);
        map.ensure(1, functor);
        EXPECT_TRUE(functorCalled);
        EXPECT_EQ(map.get(1), 1);

        functorCalled = false;
        map.ensure(2, functor);
        EXPECT_TRUE(functorCalled);
        EXPECT_EQ(map.get(2), 2);
        
        functorCalled = false;
        map.ensure(2, functor);
        EXPECT_FALSE(functorCalled);
    }
}

By the way, the CMake build is great for working on WTF because you can make and run the tests without building everything.
mkdir WebKitBuild && cd WebKitBuild && xcrun cmake .. -DPORT=Mac && make TestWTF -j8 && bin/TestWTF --gtest_filter=*Hash*

> Source/WTF/wtf/HashMap.h:193
> +        location.value = functor();

Does this never need to explicitly call the destructor of location.value?  I think you should add a test that checks if the destructor of the EmptyValue or DeletedValue is called.

> Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:557
> +

space

[Sam Weinig]

(In reply to comment #13)
> Comment on attachment 271520 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271520&action=review
> 
> Add a test that asserts that the functor is not called if not needed.
> 

The WTF_HashMap.Ensure_RefPtr test does this.

> 
> By the way, the CMake build is great for working on WTF because you can make
> and run the tests without building everything.
> mkdir WebKitBuild && cd WebKitBuild && xcrun cmake .. -DPORT=Mac && make
> TestWTF -j8 && bin/TestWTF --gtest_filter=*Hash*
> 
> > Source/WTF/wtf/HashMap.h:193
> > +        location.value = functor();
> 
> Does this never need to explicitly call the destructor of location.value?  I
> think you should add a test that checks if the destructor of the EmptyValue
> or DeletedValue is called.

No, there is no need to call the destructor in the translator. This only gets called if it is a new slot.

> 
> > Tools/TestWebKitAPI/Tests/WTF/HashMap.cpp:557
> > +
> 
> space

Will remove.

[Geoffrey Garen]

Comment on attachment 271520 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=271520&action=review

r=me

> Source/WTF/ChangeLog:34
> +            auto it = map.ensure(key, [] { return createNewValue(); });
> +            return it->value;

Do clients ever want to ensure and then use iterator? I think it would be nice if ensure returned value instead of iterator. Ensure is get + conditional add, and the union of their return values (void, and value) is value.

[Sam Weinig]

(In reply to comment #15)
> Comment on attachment 271520 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271520&action=review
> 
> r=me
> 
> > Source/WTF/ChangeLog:34
> > +            auto it = map.ensure(key, [] { return createNewValue(); });
> > +            return it->value;
> 
> Do clients ever want to ensure and then use iterator? I think it would be
> nice if ensure returned value instead of iterator. Ensure is get +
> conditional add, and the union of their return values (void, and value) is
> value.

We can certainly start with that! If people end up needing the iterator, we can change it back.

[Sam Weinig]

Committed revision 196716.