[webkit-reviews] review denied: [Bug 39802] Implement SegmentedString::lookAheadSlowCase : [Attachment 57187] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 39802: Implement SegmentedString::lookAheadSlowCase
https://bugs.webkit.org/show_bug.cgi?id=39802

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

------- Additional Comments from Darin Adler <darin at apple.com>
> +	   if (!m_pushedChar1 && string.length() <= (unsigned)
m_currentString.m_length) {

We should use C++ casts such as static_cast, not C-style casts.

> +	   if (compare(string.characters(), futureCharacters.data(),
string.length() * sizeof(UChar)))

I believe the "* sizeof(UChar)" here is incorrect.

The use of Vector<UChar> specifically for the result of the advance function
seems slightly quirky to me. The function always returns a fixed number of
characters, so paying for the range check and such as you append characters to
a vector doesn't make sense. I would have the function take a UChar* instead.

Since strings made from vectors use two malloc blocks as opposed to a single
block in the type where a vector is not involved, it would be better to use
String::createUninitialized and create a string of exactly the right size. As
long as advance doesn't have a preference for Vector<UChar> then you could use
that easily.

Do you really need to name these local comparison functions in the cryptic
memcmp style? If you're going to define your own functions anyway, maybe just
have them return booleans instead of begin memcmp-style -1,0,1 functions, and
give them normal names.

review- because of the "* sizeof(UChar)" error.