Bug 141321

Summary: Add Vector::removeFirstMatching() / removeAllMatching() methods taking lambda functions
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: Web Template FrameworkAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, benjamin, commit-queue, darin, koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 141192    
Bug Blocks: 141460    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2015-02-05 20:23:21 PST
Add Vector::removeFirstIf() / removeAllIf() methods taking lambda functions to match the element(s) to remove.
Comment 1 Chris Dumez 2015-02-05 22:10:56 PST
Created attachment 246149 [details]
Patch
Comment 2 WebKit Commit Bot 2015-02-05 22:13:10 PST
Attachment 246149 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:470:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:471:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:472:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:476:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:478:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:483:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:486:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:495:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:496:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:497:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:501:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:503:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:505:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:510:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:513:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:516:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 17 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Chris Dumez 2015-02-06 09:20:43 PST
Created attachment 246163 [details]
Patch
Comment 4 Anders Carlsson 2015-02-06 09:25:05 PST
Comment on attachment 246163 [details]
Patch

I don't like the name removeAllIf; it sounds like it'll clear the vector if the lambda returns true.
Comment 5 Chris Dumez 2015-02-06 09:26:52 PST
(In reply to comment #4)
> Comment on attachment 246163 [details]
> Patch
> 
> I don't like the name removeAllIf; it sounds like it'll clear the vector if
> the lambda returns true.

No argument there. We need to find better names for removeFirst(value) / removeFirstIf(lambda), removeAll(value) / removeAllIf(lambda). Do you have any suggestion?
Comment 6 WebKit Commit Bot 2015-02-06 09:30:12 PST
Attachment 246163 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:470:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:471:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:472:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:476:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:478:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:483:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:486:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:495:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:496:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:497:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:501:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:503:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:505:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:510:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:513:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:516:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 17 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Chris Dumez 2015-02-06 16:03:34 PST
I have the following naming proposal:
- removeAt(index)
- removeSingle(value) // first occurrence
- removeSingleIf(lambda) // first match
- remove(value) // every occurrence
- removeIf(lamba) // every match

Any thoughts?
Comment 8 Chris Dumez 2015-02-07 11:12:55 PST
(In reply to comment #7)
> I have the following naming proposal:
> - removeAt(index)
> - removeSingle(value) // first occurrence
> - removeSingleIf(lambda) // first match
> - remove(value) // every occurrence
> - removeIf(lamba) // every match
> 
> Any thoughts?

Darin, what do you think about this naming? Do you have a better proposal?
Comment 9 Darin Adler 2015-02-07 11:36:53 PST
I am trying to find some prior art. I think I prefer "First" to "Single" but there is something that rubs me the wrong way about using "if" to mean "based on a predicate", because "if" sounds like it’s the entire function that’s conditional, not just something the function does.

I would never say “remove all if equal to 5”, I would say “remove all that are equal to 5” or “remove all if there was a problem”.

I am trying to find some prior art on naming functions that take predicates. Even better if it was in a project that used a sort of “literate” naming style reminiscent to the one from Cocoa, which inspires a lot of our function naming in WebKit.
Comment 10 Darin Adler 2015-02-07 11:39:49 PST
Cocoa uses the words “using block” and “passing test” in their NSArray functions. Neither seems terse enough for our purposes.
Comment 11 Chris Dumez 2015-02-07 11:43:06 PST
(In reply to comment #9)
> I am trying to find some prior art. I think I prefer "First" to "Single" but
> there is something that rubs me the wrong way about using "if" to mean
> "based on a predicate", because "if" sounds like it’s the entire function
> that’s conditional, not just something the function does.
> 
> I would never say “remove all if equal to 5”, I would say “remove all that
> are equal to 5” or “remove all if there was a problem”.
> 
> I am trying to find some prior art on naming functions that take predicates.
> Even better if it was in a project that used a sort of “literate” naming
> style reminiscent to the one from Cocoa, which inspires a lot of our
> function naming in WebKit.

Well, STL has remove_if:
http://en.cppreference.com/w/cpp/algorithm/remove

I agree that removeAllIf() is confusing, which is why my latest proposal is removeIf().
Comment 12 Chris Dumez 2015-02-07 15:52:52 PST
For the lambda function, we could also use a method name with "Matching" in the name, e.g.:
removeFirstMatching(lambda) / removeAllMatching(lambda)
Comment 13 Chris Dumez 2015-02-07 16:02:56 PST
Created attachment 246224 [details]
Patch
Comment 14 WebKit Commit Bot 2015-02-07 16:04:21 PST
Attachment 246224 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:470:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:471:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:472:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:476:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:478:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:483:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:486:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:495:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:496:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:497:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:501:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:503:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:505:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:510:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:513:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Tools/TestWebKitAPI/Tests/WTF/Vector.cpp:516:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 17 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Darin Adler 2015-02-07 17:13:39 PST
Comment on attachment 246224 [details]
Patch

I like these “matching” names. And they are nice and long and easy to globally replace if we get another naming idea.
Comment 16 Anders Carlsson 2015-02-07 17:48:04 PST
(In reply to comment #15)
> Comment on attachment 246224 [details]
> Patch
> 
> I like these “matching” names. And they are nice and long and easy to
> globally replace if we get another naming idea.

I agree.
Comment 17 Chris Dumez 2015-02-07 17:48:38 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Comment on attachment 246224 [details]
> > Patch
> > 
> > I like these “matching” names. And they are nice and long and easy to
> > globally replace if we get another naming idea.
> 
> I agree.

\o/
Comment 18 WebKit Commit Bot 2015-02-07 18:28:54 PST
Comment on attachment 246224 [details]
Patch

Clearing flags on attachment: 246224

Committed r179791: <http://trac.webkit.org/changeset/179791>
Comment 19 WebKit Commit Bot 2015-02-07 18:29:00 PST
All reviewed patches have been landed.  Closing bug.