Bug 187963 – String(View) should have a splitAllowingEmptyEntries function instead of a flag parameter

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Ross Kirsling]

Based on https://bugs.webkit.org/show_bug.cgi?id=187864#c19.

Instead of letting WTF::String::split and WTF::StringView::split take an allowEmptyEntries boolean or enum value, it would be better practice to have a separate splitWithEmptyEntries method on each of these classes.

[Ross Kirsling]

Created attachment 345722 [details]
Patch

[Ross Kirsling]

Although the update to StringView is trivial, the update to String is slightly less so -- please let me know if there's a better practice here than the approach I've taken with splitInternal.

[Ross Kirsling]

Created attachment 345730 [details]
Patch

[Sam Weinig]

Comment on attachment 345722 [details]
Patch

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

> Source/WTF/wtf/text/StringView.h:129
> +    SplitResult splitWithEmptyEntries(UChar) const;

I think I would call this something like splitAllowingEmptyEntries()

> Source/WTF/wtf/text/WTFString.cpp:705
> +void String::splitInternal(const String& separator, bool allowEmptyEntries, Vector<String>& result) const

I think following a pattern like String::removeInternal, and making this a template function (templatized on allowEmptyEntries).  That will probably also require out-of-lining all the String::split() functions that call String::splitInternal().

> Source/WTF/wtf/text/WTFString.h:285
> +    void splitWithEmptyEntries(const String& separator, Vector<String>& result) const { splitInternal(separator, true, result); }
> +    void splitWithEmptyEntries(UChar separator, Vector<String>& result) const { splitInternal(separator, true, result); }
> +    void splitWithEmptyEntries(UChar separator, const SplitFunctor& functor) const { splitInternal(separator, true, functor); }
> +    Vector<String> splitWithEmptyEntries(UChar separator) const;
> +    Vector<String> splitWithEmptyEntries(const String& separator) const;

Again, I would probably go with splitAllowingEmptyEntries

[Ross Kirsling]

Created attachment 345744 [details]
Patch

[Alex Christensen]

Comment on attachment 345744 [details]
Patch

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

Cool!

> Source/WTF/wtf/text/WTFString.h:282
> +    WTF_EXPORT_PRIVATE void splitAllowingEmptyEntries(const String& separator, Vector<String>& result) const;
> +    WTF_EXPORT_PRIVATE void splitAllowingEmptyEntries(UChar separator, Vector<String>& result) const;

Let's get rid of these return-by-reference functions in favor of the return-by-value functions.  All uses of them create a vector on the line before calling them.

> Source/WTF/wtf/text/WTFString.h:285
> +    Vector<String> splitAllowingEmptyEntries(UChar separator) const;
> +    Vector<String> splitAllowingEmptyEntries(const String& separator) const;

Ideally even these would just return an iterator of some kind so we don't need to allocate a Vector, because when we use this we really want to iterate it quickly.  That might be better for a future patch, though.

> Tools/ChangeLog:9
> +        Add tests for String::split and String::splitWithEmptyEntries.

String::splitAllowingEmptyEntries

[Ross Kirsling]

(In reply to Alex Christensen from comment #6)
> Comment on attachment 345744 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=345744&action=review
> 
> Cool!
> 
> > Source/WTF/wtf/text/WTFString.h:282
> > +    WTF_EXPORT_PRIVATE void splitAllowingEmptyEntries(const String& separator, Vector<String>& result) const;
> > +    WTF_EXPORT_PRIVATE void splitAllowingEmptyEntries(UChar separator, Vector<String>& result) const;
> 
> Let's get rid of these return-by-reference functions in favor of the
> return-by-value functions.  All uses of them create a vector on the line
> before calling them.

Can do!

> > Source/WTF/wtf/text/WTFString.h:285
> > +    Vector<String> splitAllowingEmptyEntries(UChar separator) const;
> > +    Vector<String> splitAllowingEmptyEntries(const String& separator) const;
> 
> Ideally even these would just return an iterator of some kind so we don't
> need to allocate a Vector, because when we use this we really want to
> iterate it quickly.  That might be better for a future patch, though.

Sure! Are we okay with with mimicking StringView::SplitResult::Iterator for String then?
https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/text/StringView.h#L689-L711

[Darin Adler]

(In reply to Ross Kirsling from comment #7)
> > > Source/WTF/wtf/text/WTFString.h:285
> > > +    Vector<String> splitAllowingEmptyEntries(UChar separator) const;
> > > +    Vector<String> splitAllowingEmptyEntries(const String& separator) const;
> > 
> > Ideally even these would just return an iterator of some kind so we don't
> > need to allocate a Vector, because when we use this we really want to
> > iterate it quickly.  That might be better for a future patch, though.
> 
> Sure! Are we okay with with mimicking StringView::SplitResult::Iterator for
> String then?
> https://github.com/WebKit/webkit/blob/master/Source/WTF/wtf/text/StringView.
> h#L689-L711

I think instead we should eventually move all clients from the String::split functions to StringView::split functions and just dump these ones that return a Vector. Any caller can use the more efficient StringView version.

I do think that we should make some other small fixes to call sites, such as getting rid of pointless UChar casts and also fixing call sites that get the less efficient version by passing "x" instead of 'x'. I also think that as long as we are keeping them, we should consider changing String::split and String::splitAllowingEmptyEntries to take a StringView for the separator argument rather than a const String&. There is no reason to require an actual String object.

[Ross Kirsling]

Created attachment 345832 [details]
Patch

[Ross Kirsling]

For now, I've removed the out-parameter overloads and adjusted all call sites accordingly.
I've also removed needless UChar casts, ensured that character (not string) literals are used, and simplified for loops where trivial.

It seemed for a moment that the const String& overloads weren't even being used, but unfortunately, WebSocket::subprotocolSeparator is ", ".
I tried switching to StringView, but this would demand that a similar change be made to String::find, so it seems out of scope for this patch.

[WebKit Commit Bot]

Comment on attachment 345832 [details]
Patch

Clearing flags on attachment: 345832

Committed r234278: <https://trac.webkit.org/changeset/234278>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/42639510>