[webkit-reviews] review granted: [Bug 100576] Non-special escape character sequences cause JSC::Lexer::parseString to create 16 bit strings : [Attachment 171285] Patch with suggested updates

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Michael Saboff <msaboff at apple.com>'s
request for review:
Bug 100576: Non-special escape character sequences cause
JSC::Lexer::parseString to create 16 bit strings
https://bugs.webkit.org/show_bug.cgi?id=100576

Attachment 171285: Patch with suggested updates
https://bugs.webkit.org/attachment.cgi?id=171285&action=review

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


> Source/JavaScriptCore/ChangeLog:16
> +	   (JSC::::Lexer):

?

> Source/JavaScriptCore/ChangeLog:19
> +	   (JSC::::parseString):
> +	   (JSC::::parseStringSlowCase):

Should be:

    (JSC::Lexer::parseString):
    (JSC::Lexer::parseStringSlowCase):

The script gets it wrong, but you could fix it.

> Source/JavaScriptCore/parser/Lexer.cpp:657
> +    if ((c >= 32) && (c <= 127)) {
> +	   ASSERT(static_cast<size_t>(c - 32) <
ARRAY_SIZE(singleCharacterEscapeValuesForASCII));
> +	   return singleCharacterEscapeValuesForASCII[c - 32];
>      }
> +    return 0;

Why not save two branches by putting a bunch of zeros in for 0..31 and
128..255?

> LayoutTests/ChangeLog:8
> +	   Added a new test to validated the behaviorC of the corresponding
changes to string processing

Typo: behaviorC

>
LayoutTests/fast/js/normal-character-escapes-in-string-literals-expected.txt:88

> +PASS eval('"\Â "') is "Â "
> +PASS eval('"\£"') is "£"
> +PASS eval('"\«"') is "«"
> +PASS eval('"\´"') is "´"

All the others are great, but these end up a little ugly.

>
LayoutTests/fast/js/script-tests/normal-character-escapes-in-string-literals.js
:8
> +    escapedCharacter = '\\' + character;
> +    expectedResult = character;

No need for these local variables.

>
LayoutTests/fast/js/script-tests/normal-character-escapes-in-string-literals.js
:9
> +    shouldBeEqualToString('eval(\'"' + escapedCharacter + '"\')',
expectedResult);

Could be:
 
    shouldBeEqualToString('eval(\'"\\' + character + '"\')', character);