Bug 85154 - Add String::startsWith() and endsWith() for string literals
Summary: Add String::startsWith() and endsWith() for string literals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-28 23:35 PDT by Benjamin Poulain
Modified: 2012-04-30 14:32 PDT (History)
7 users (show)

See Also:


Attachments
Patch (20.96 KB, patch)
2012-04-29 00:22 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (21.13 KB, patch)
2012-04-29 00:47 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Patch (24.65 KB, patch)
2012-04-29 01:51 PDT, Benjamin Poulain
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2012-04-28 23:35:15 PDT
Add String::startsWith() and endsWith() for string literals
Comment 1 Benjamin Poulain 2012-04-29 00:22:17 PDT
Created attachment 139396 [details]
Patch
Comment 2 Gustavo Noronha (kov) 2012-04-29 00:30:14 PDT
Comment on attachment 139396 [details]
Patch

Attachment 139396 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12557841
Comment 3 Build Bot 2012-04-29 00:37:27 PDT
Comment on attachment 139396 [details]
Patch

Attachment 139396 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12557838
Comment 4 Early Warning System Bot 2012-04-29 00:38:39 PDT
Comment on attachment 139396 [details]
Patch

Attachment 139396 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12559830
Comment 5 Early Warning System Bot 2012-04-29 00:40:05 PDT
Comment on attachment 139396 [details]
Patch

Attachment 139396 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12550823
Comment 6 Benjamin Poulain 2012-04-29 00:47:51 PDT
Created attachment 139397 [details]
Patch
Comment 7 Benjamin Poulain 2012-04-29 01:51:12 PDT
Created attachment 139398 [details]
Patch
Comment 8 Darin Adler 2012-04-29 10:43:19 PDT
Comment on attachment 139398 [details]
Patch

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.
Comment 9 Benjamin Poulain 2012-04-29 12:36:22 PDT
Thanks for the review. I should use a spellchecker for my change logs...

> > 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.

They are not, I just figured that could be useful in the future.
Comment 10 Benjamin Poulain 2012-04-30 14:32:01 PDT
Committed r115669: <http://trac.webkit.org/changeset/115669>