[webkit-reviews] review granted: [Bug 175757] StringView could use a function to strip leading/trailing characters without allocation : [Attachment 318609] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 175757: StringView could use a function to strip leading/trailing
characters without allocation
https://bugs.webkit.org/show_bug.cgi?id=175757

Attachment 318609: Patch

https://bugs.webkit.org/attachment.cgi?id=318609&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 318609
  --> https://bugs.webkit.org/attachment.cgi?id=318609
Patch

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

I have no objection to adding this.

My concern is about stripping one WTF::String and storing the result in another
WTF::String or WTF::AtomicString. When there is nothing to strip, it’s
important that we not convert to a StringView and then call toString since it
will always copy the string, or call toAtomicString which will have to look up
the table in the atomic string table even though it hasn’t been changed.

When stripping a WTF::String or WTF::StringView and then processing the result,
it’s great to have a function like this one!

> Source/WTF/ChangeLog:21
> +	   For instance, the check (from ScriptElement.cpp:287):
> +
> +	   if (!stripLeadingAndTrailingHTMLSpaces(sourceURL).isEmpty()) {
> +	       ...
> +	   }
> +
> +	   currently allocates a string just to make this check. With a new 
> +	   stripLeadingAndTrailingHTMLSpaces such as:

I think it would be nice to have the above call site use a “contains” function
rather than a “strip” function. I don’t find “strip.isEmpty” to be a clear
idiom.

> Source/WTF/wtf/text/StringView.h:123
> +    template<typename MatchedCharacterPredicate>
> +    StringView
stripLeadingAndTrailingMatchedCharacters(MatchedCharacterPredicate&&);

I don’t understand why this takes an rvalue reference to the predicate. It
seems the function will call the predicate multiple times. That seems like
"const&", not "&&", which we normally would use when a function will take
ownership of an object or might take ownership of an object.

> Source/WTF/wtf/text/StringView.h:967
> +    if (!start && end == m_length - 1)
> +	   return *this;
> +
> +    StringView result(characters + start, end + 1 - start);
> +    result.setUnderlyingString(*this);
> +    return result;

Why not call StringView::substring and keep this function simpler and shorter?