Bug 238287 - String's find() / reverseFind() / replace() should take in a StringView instead of a String
Summary: String's find() / reverseFind() / replace() should take in a StringView inste...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-03-23 13:54 PDT by Chris Dumez
Modified: 2022-03-24 10:14 PDT (History)
32 users (show)

See Also:

Attachments
Patch (35.26 KB, patch)
2022-03-23 13:58 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (37.24 KB, patch)
2022-03-23 14:17 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (38.36 KB, patch)
2022-03-23 15:58 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (56.93 KB, patch)
2022-03-23 19:27 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (57.84 KB, patch)
2022-03-23 21:09 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (57.84 KB, patch)
2022-03-23 21:47 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.85 KB, patch)
2022-03-23 22:15 PDT, Chris Dumez 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (56.85 KB, patch)
2022-03-23 23:03 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (72.23 KB, patch)
2022-03-24 08:28 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (72.25 KB, patch)
2022-03-24 08:36 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (9) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2022-03-23 13:54:17 PDT 
String's find() / reverseFind() / replace() should take in a StringView instead of a String, to avoid unnecessary String construction.
Comment 1 Chris Dumez 2022-03-23 13:58:48 PDT 
Created attachment 455549 [details]
Patch
Comment 2 Chris Dumez 2022-03-23 14:17:30 PDT 
Created attachment 455552 [details]
Patch
Comment 3 Chris Dumez 2022-03-23 15:58:55 PDT 
Created attachment 455573 [details]
Patch
Comment 4 Chris Dumez 2022-03-23 19:27:12 PDT 
Created attachment 455598 [details]
Patch
Comment 5 Chris Dumez 2022-03-23 21:09:39 PDT 
Created attachment 455601 [details]
Patch
Comment 6 Chris Dumez 2022-03-23 21:47:01 PDT 
Created attachment 455604 [details]
Patch
Comment 7 Chris Dumez 2022-03-23 22:15:39 PDT 
Created attachment 455607 [details]
Patch
Comment 8 Chris Dumez 2022-03-23 23:03:40 PDT 
Created attachment 455610 [details]
Patch
Comment 9 Darin Adler 2022-03-24 08:13:37 PDT 
Comment on attachment 455610 [details]
Patch

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

> Source/WTF/wtf/text/AtomString.h:124
> +    size_t findIgnoringASCIICase(StringView, unsigned startOffset) const;

Strange that we call it "start" in find and call it "startOffset" in findIgnoringASCIICase. Also strange that one uses a defaulted argument and the other uses overloading.

> Source/WTF/wtf/text/StringImpl.cpp:964
>      if (UNLIKELY(!matchString))

I typically like to write isNull() rather than depending on operator!, but I suppose both work?

> Source/WTF/wtf/text/StringImpl.h:436
> +    WTF_EXPORT_PRIVATE size_t find(StringView, unsigned index);

I find the mix of "index", "start", and "startOffset" slightly annoying.

> Source/WebCore/page/UserContentURLPattern.cpp:66
>      static NeverDestroyed<const String> schemeSeparator(MAKE_STATIC_STRING_IMPL("://"));

This is now never used as a String, so I suggest we just make it an ASCIILiteral or a const char* or an auto. No need for NeverDestroyed any more.

> Source/WebCore/platform/network/curl/CurlMultipartHandle.cpp:55
> +        auto splitPosistion = header.find(':');

Noticed the "posistion" typo here.

> Source/WebKit/NetworkProcess/soup/NetworkProcessSoup.cpp:58
> +    size_t cLocalePosition = languages.find("c"_s);

Even this can be changed so it doesn’t involve creating/destroying strings. No reason Vector::find can’t be changed to work with anything that can do == with the Vector elements, doesn’t need to actually be the same type as the element.

Not that you should do this right now.
Comment 10 Chris Dumez 2022-03-24 08:21:10 PDT 
(In reply to Darin Adler from comment #9)
> Comment on attachment 455610 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=455610&action=review
> 
> > Source/WTF/wtf/text/AtomString.h:124
> > +    size_t findIgnoringASCIICase(StringView, unsigned startOffset) const;
> 
> Strange that we call it "start" in find and call it "startOffset" in
> findIgnoringASCIICase. Also strange that one uses a defaulted argument and
> the other uses overloading.
> 
> > Source/WTF/wtf/text/StringImpl.cpp:964
> >      if (UNLIKELY(!matchString))
> 
> I typically like to write isNull() rather than depending on operator!, but I
> suppose both work?

Yes, it is identical:
```
inline StringView::operator bool() const
{
    return !isNull();
}
```
Comment 11 Chris Dumez 2022-03-24 08:28:33 PDT 
Created attachment 455641 [details]
Patch
Comment 12 Chris Dumez 2022-03-24 08:36:03 PDT 
Created attachment 455642 [details]
Patch
Comment 13 Chris Dumez 2022-03-24 09:55:54 PDT 
Comment on attachment 455642 [details]
Patch

Clearing flags on attachment: 455642

Committed r291800 (248828@trunk): <https://commits.webkit.org/248828@trunk>
Comment 14 Chris Dumez 2022-03-24 09:55:59 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2022-03-24 09:56:18 PDT 
<rdar://problem/90774749>