[webkit-reviews] review granted: [Bug 85154] Add String::startsWith() and endsWith() for string literals : [Attachment 139398] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 85154: Add String::startsWith() and endsWith() for string literals
https://bugs.webkit.org/show_bug.cgi?id=85154

Attachment 139398: Patch
https://bugs.webkit.org/attachment.cgi?id=139398&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139398&action=review


> Source/WTF/ChangeLog:9
> +	   a string litteral, a new String was constructed implicitely,
allocating

litteral -> literal
implicitely -> implicitly

> Source/WTF/ChangeLog:10
> +	   a new StringImpl and copying the caracters for the operation.

caracters -> characters

> Source/WTF/ChangeLog:12
> +	   This patch adds a version of those methods for single character and

single character -> single characters
litterals -> literals

> Source/WTF/wtf/text/AtomicString.h:87
> +    bool startsWith(const char* s, unsigned matchLength, bool caseSensitive
= true) const
> +	   { return m_string.startsWith(s, matchLength, caseSensitive); }

Is this overload used anywhere?

> Source/WTF/wtf/text/AtomicString.h:97
> +    bool endsWith(const char* s, unsigned matchLength, bool caseSensitive =
true) const
> +	   { return m_string.endsWith(s, matchLength, caseSensitive); }

Same question.

> Source/WTF/wtf/text/StringImpl.cpp:1118
> +    if (length())
> +	   return this->operator[](0) == character;
> +    return false;

I would write this differently:

    return m_length && (*this)[0] == character;

> Source/WTF/wtf/text/StringImpl.cpp:1143
> +    if (length())
> +	   return this->operator[](length() - 1) == character;
> +    return false;

I would write this differently:

    return m_length && (*this)[m_length - 1] == character;

> Source/WTF/wtf/text/StringImpl.cpp:1153
> +    ASSERT(matchLength);
> +    if (matchLength > length())
> +	   return false;
> +    unsigned startOffset = length() - matchLength;
> +
> +    return equalInner(this, startOffset, matchString, matchLength,
caseSensitive);

The paragraphing here seems strange. I’d leave out the blank line.