[webkit-reviews] review denied: [Bug 67337] Update RegExp and related classes to use 8 bit strings when available : [Attachment 106671] Proposed Patch

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 67337: Update RegExp and related classes to use 8 bit strings when
available
https://bugs.webkit.org/show_bug.cgi?id=67337

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

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=106671&action=review


Looks good. Few issues above, my only major concern is is branch16, but would
be prudent to get some non-JIT numbers to prove the extra check in the
interpreter isn't an overhead.

> Source/JavaScriptCore/assembler/MacroAssemblerMIPS.h:1074
> +    Jump branch16(RelationalCondition cond, RegisterID left, TrustedImm32
right)

This is actually doing a 32-bit by 16-bit compare.  We should either make this
work like the other branch16 method (only consider the low 16 bits), or we
should be ripping out branch16 and just using a branch32.  A method called
branch16 that compares 32 bits isn't right.

> Source/JavaScriptCore/assembler/MacroAssemblerX86Common.h:884
> +	       m_assembler.testl_rr(left, left);

I think this needs to be a testw?

> Source/JavaScriptCore/runtime/RegExp.cpp:308
> +{

It might help understanding to add the following documentary ASSERT:

// If the state is NotCompiled or ParseError, then there is no representation.
// If there is a representation, and the state must be either JITCode or
ByteCode.
ASSERT(!!m_representation == (m_state == JITCode || m_state == ByteCode));

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:170
>  

We should probably have some kind of comment on this class indicating that it
is a placeholder for something better one we have 8-bit StringImpls.

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:209
> +	       if (m_charSize == Char8)

We should check the performance impact of this check.