RESOLVED FIXED 134857
Add an ensure function on HashMap that takes a key and a function to make the lazy value initialization idiom easier
https://bugs.webkit.org/show_bug.cgi?id=134857
Summary Add an ensure function on HashMap that takes a key and a function to make the...
Sam Weinig
Reported 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
Attachments
Patch (9.08 KB, patch)
2014-07-12 14:18 PDT, Sam Weinig
no flags
Patch (9.18 KB, patch)
2014-07-12 15:10 PDT, Sam Weinig
no flags
Patch (9.08 KB, patch)
2014-07-12 15:35 PDT, Sam Weinig
no flags
Patch (9.13 KB, patch)
2014-07-12 15:48 PDT, Sam Weinig
no flags
Patch (8.86 KB, patch)
2014-07-12 16:59 PDT, Sam Weinig
no flags
Patch (7.77 KB, patch)
2016-02-16 19:36 PST, Sam Weinig
ggaren: review+
Sam Weinig
Comment 1 2014-07-12 14:18:25 PDT
Sam Weinig
Comment 2 2014-07-12 15:10:15 PDT
WebKit Commit Bot
Comment 3 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.
Sam Weinig
Comment 4 2014-07-12 15:35:07 PDT
WebKit Commit Bot
Comment 5 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.
Sam Weinig
Comment 6 2014-07-12 15:48:04 PDT
WebKit Commit Bot
Comment 7 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.
Sam Weinig
Comment 8 2014-07-12 16:59:05 PDT
WebKit Commit Bot
Comment 9 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.
Sam Weinig
Comment 10 2016-02-16 19:36:24 PST
WebKit Commit Bot
Comment 11 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.
Sam Weinig
Comment 12 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.
Alex Christensen
Comment 13 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
Sam Weinig
Comment 14 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.
Geoffrey Garen
Comment 15 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.
Sam Weinig
Comment 16 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.
Sam Weinig
Comment 17 2016-02-17 13:17:03 PST
Committed revision 196716.
Note You need to log in before you can comment on or make changes to this bug.