Bug 238287 – String's find() / reverseFind() / replace() should take in a StringView instead of a String

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

String's find() / reverseFind() / replace() should take in a StringView instead of a String, to avoid unnecessary String construction.

[Chris Dumez]

Created attachment 455549 [details]
Patch

[Chris Dumez]

Created attachment 455552 [details]
Patch

[Chris Dumez]

Created attachment 455573 [details]
Patch

[Chris Dumez]

Created attachment 455598 [details]
Patch

[Chris Dumez]

Created attachment 455601 [details]
Patch

[Chris Dumez]

Created attachment 455604 [details]
Patch

[Chris Dumez]

Created attachment 455607 [details]
Patch

[Chris Dumez]

Created attachment 455610 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

(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();
}
```

[Chris Dumez]

Created attachment 455641 [details]
Patch

[Chris Dumez]

Created attachment 455642 [details]
Patch

[Chris Dumez]

Comment on attachment 455642 [details]
Patch

Clearing flags on attachment: 455642

Committed r291800 (248828@trunk): <https://commits.webkit.org/248828@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/90774749>