WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Sam Weinig
Comment 1
2014-07-12 14:18:25 PDT
Created
attachment 234806
[details]
Patch
Sam Weinig
Comment 2
2014-07-12 15:10:15 PDT
Created
attachment 234808
[details]
Patch
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
Created
attachment 234809
[details]
Patch
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
Created
attachment 234810
[details]
Patch
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
Created
attachment 234813
[details]
Patch
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
Created
attachment 271520
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug