[webkit-reviews] review denied: [Bug 63340] Lexer error messages are not very informative : [Attachment 99335] proposed patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Juan C. Montemayor
<jmont at apple.com>'s request for review:
Bug 63340: Lexer error messages are not very informative
https://bugs.webkit.org/show_bug.cgi?id=63340

Attachment 99335: proposed patch
https://bugs.webkit.org/attachment.cgi?id=99335&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=99335&action=review

Looking good -- the new error messages are a huge improvement. But I think this
patch could be even better.

> Source/JavaScriptCore/parser/Lexer.cpp:237
> +ALWAYS_INLINE UString Lexer::getInvalidCharMessage()

I don't think you want to inline this -- it's the error case, so it should be
out of line, for smaller code generation and less register pressure and
instruction cache pollution in the non-error path.

> Source/JavaScriptCore/parser/Lexer.cpp:241
> +	   return UString("Invalid character '\\0'").impl();

Since your return type is UString, you can just return a C string
("Invalid..."), and the UString constructor will do the rest. This construct
you've used will create two UStrings for every call, instead of one.

> Source/JavaScriptCore/parser/Lexer.cpp:243
> +	   return UString("Invalid character '\\n'").impl();

I think these messages would be clearer with a colon: "Invalid character:
...").

> Source/JavaScriptCore/parser/Lexer.cpp:554
> +		       m_lexErrorMessage = "\\u can only be followed by a
unicode character sequence";

Unicode is a proper noun, so please capitalize it.

> Source/JavaScriptCore/parser/Lexer.cpp:562
> +		       m_lexErrorMessage = "Only valid numeric escape in strict
mode is '\\0'";

To make this a complete sentence, say "The only valid..."

> Source/JavaScriptCore/parser/Lexer.cpp:1296
> +    m_lexErrorMessage = UString("Lexer error");

No need for "UString" here -- the constructor will do that for you.