Bug 234855 - Add FixedVector::clear(), contains(), find(), and findMatching().
Summary: Add FixedVector::clear(), contains(), find(), and findMatching().
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 234864
  Show dependency treegraph
 
Reported: 2022-01-04 11:57 PST by Mark Lam
Modified: 2022-01-24 13:00 PST (History)
12 users (show)

See Also:


Attachments
proposed patch. (9.45 KB, patch)
2022-01-04 12:22 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2022-01-04 11:57:45 PST
This is needed so that we can use FixedVector in DeferredWorkTimer (see https://bugs.webkit.org/show_bug.cgi?id=234628#c2).
Comment 1 Mark Lam 2022-01-04 12:22:24 PST
Created attachment 448318 [details]
proposed patch.
Comment 2 Yusuke Suzuki 2022-01-04 12:55:22 PST
Comment on attachment 448318 [details]
proposed patch.

r=me
Comment 3 Geoffrey Garen 2022-01-04 15:06:05 PST
Comment on attachment 448318 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=448318&action=review

> Source/WTF/wtf/FixedVector.h:186
> +size_t FixedVector<T>::findMatching(const MatchFunction& matches) const

FWIW, the standard library calls this operation "findIf", which I find slightly clearer. ("Matching" sort of implies that you are searching for 'matches' as an entry in the subject vector.)
Comment 4 Mark Lam 2022-01-04 15:55:05 PST
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 448318 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448318&action=review
> 
> > Source/WTF/wtf/FixedVector.h:186
> > +size_t FixedVector<T>::findMatching(const MatchFunction& matches) const
> 
> FWIW, the standard library calls this operation "findIf", which I find
> slightly clearer. ("Matching" sort of implies that you are searching for
> 'matches' as an entry in the subject vector.)

The findMatching name is a pre-existing one from WTF::Vector, and FixedVector aims to match Vector's API.  It looks like it is used all over the place.  I'll leave it as is for now so that we don't pollute this patch.  We can do a global rename in a separate patch later.
Comment 5 Mark Lam 2022-01-04 16:09:18 PST
(In reply to Mark Lam from comment #4)
> The findMatching name is a pre-existing one from WTF::Vector, and
> FixedVector aims to match Vector's API.  It looks like it is used all over
> the place.  I'll leave it as is for now so that we don't pollute this patch.
> We can do a global rename in a separate patch later.

Will do this renaming in https://bugs.webkit.org/show_bug.cgi?id=234864.
Comment 6 Radar WebKit Bug Importer 2022-01-11 11:58:17 PST
<rdar://problem/87412124>
Comment 7 Mark Lam 2022-01-24 11:03:57 PST
Comment on attachment 448318 [details]
proposed patch.

Thanks for the review.  Landing now.
Comment 8 EWS 2022-01-24 12:02:21 PST
Committed r288458 (246344@main): <https://commits.webkit.org/246344@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448318 [details].
Comment 9 Darin Adler 2022-01-24 13:00:16 PST
Comment on attachment 448318 [details]
proposed patch.

I think that adding contains, findMatching, and find, are small steps in the wrong direction.

All of these work well on a Span, no need to have them as member functions on Vector. We need to find a way to not replicate the huge number of Vector functions all on FixedVector, by changing them to non-member functions with Span arguments. Both Vector and FixedVector can be efficiently changed into a Span, which can be thought of as a sort of VectorView.

If we convert most const functions in Vector to work on Span, then we can use them on C arrays (possibly), std::array, any Span, Vector, FixedVector, or even other things we invent in the future.

I think that eventually we should be doing that with one function at a time, rather than replicating all the Vector functions one at a time.