Bug 134857 - Add an ensure function on HashMap that takes a key and a function to make the lazy value initialization idiom easier
Summary: Add an ensure function on HashMap that takes a key and a function to make the...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sam Weinig
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-12 14:00 PDT by Sam Weinig
Modified: 2016-02-17 13:17 PST (History)
5 users (show)

See Also:


Attachments
Patch (9.08 KB, patch)
2014-07-12 14:18 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (9.18 KB, patch)
2014-07-12 15:10 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2014-07-12 15:35 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (9.13 KB, patch)
2014-07-12 15:48 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (8.86 KB, patch)
2014-07-12 16:59 PDT, Sam Weinig
no flags Details | Formatted Diff | Diff
Patch (7.77 KB, patch)
2016-02-16 19:36 PST, Sam Weinig
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2014-07-12 14:00:25 PDT
Add an overload of HashMap::add that takes a function instead of the value to make the lazy value initialization idiom easier
Comment 1 Sam Weinig 2014-07-12 14:18:25 PDT
Created attachment 234806 [details]
Patch
Comment 2 Sam Weinig 2014-07-12 15:10:15 PDT
Created attachment 234808 [details]
Patch
Comment 3 WebKit Commit Bot 2014-07-12 15:11:49 PDT
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.
Comment 4 Sam Weinig 2014-07-12 15:35:07 PDT
Created attachment 234809 [details]
Patch
Comment 5 WebKit Commit Bot 2014-07-12 15:36:52 PDT
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.
Comment 6 Sam Weinig 2014-07-12 15:48:04 PDT
Created attachment 234810 [details]
Patch
Comment 7 WebKit Commit Bot 2014-07-12 15:49:46 PDT
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.
Comment 8 Sam Weinig 2014-07-12 16:59:05 PDT
Created attachment 234813 [details]
Patch
Comment 9 WebKit Commit Bot 2014-07-12 17:00:19 PDT
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.
Comment 10 Sam Weinig 2016-02-16 19:36:24 PST
Created attachment 271520 [details]
Patch
Comment 11 WebKit Commit Bot 2016-02-16 19:37:01 PST
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.
Comment 12 Sam Weinig 2016-02-16 19:37:51 PST
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.
Comment 13 Alex Christensen 2016-02-16 22:40:20 PST
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
Comment 14 Sam Weinig 2016-02-17 06:28:35 PST
(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.
Comment 15 Geoffrey Garen 2016-02-17 10:00:02 PST
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.
Comment 16 Sam Weinig 2016-02-17 10:20:16 PST
(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.
Comment 17 Sam Weinig 2016-02-17 13:17:03 PST
Committed revision 196716.