Add an overload of HashMap::add that takes a function instead of the value to make the lazy value initialization idiom easier
Created attachment 234806 [details] Patch
Created attachment 234808 [details] Patch
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.
Created attachment 234809 [details] Patch
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.
Created attachment 234810 [details] Patch
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.
Created attachment 234813 [details] Patch
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.
Created attachment 271520 [details] Patch
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.
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 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
(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 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.
(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.
Committed revision 196716.