Bug 39802 – Implement SegmentedString::lookAheadSlowCase

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

Implement SegmentedString::lookAheadSlowCase

[Adam Barth]

Created attachment 57184 [details]
Work in progress

[Adam Barth]

Created attachment 57187 [details]
Patch

[Darin Adler]

Comment on attachment 57187 [details]
Patch

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

[Adam Barth]

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

Good catch.  That snuck in because of an earlier iteration.

[Adam Barth]

Created attachment 57191 [details]
Patch

Thanks to Darin, we now pass the doctype suite in the resumer harness.  I haven't turned on the test because it fails with the old parser.

[Darin Adler]

Comment on attachment 57191 [details]
Patch

> +        *consumedCharacters++ = *current();

Just a nitpick: I would write:

    consumedCharacters[i] = *current();

But the two are equivalent.

> +        if (string.length() > length())
> +            return NotEnoughCharacters;
> +        unsigned count = string.length();

If you set up count one line earlier then you wouldn't have to evaluate string.length() twice.

> +        UChar* consumedCharacters = 0;

No need to initialize to zero here.

r=me

[Adam Barth]

Created attachment 57198 [details]
Patch for landing

[Adam Barth]

Comment on attachment 57198 [details]
Patch for landing

Clearing flags on attachment: 57198

Committed r60274: <http://trac.webkit.org/changeset/60274>

[Adam Barth]

All reviewed patches have been landed.  Closing bug.