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

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver 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 99397: revised patch
https://bugs.webkit.org/attachment.cgi?id=99397&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=99397&action=review

> Source/JavaScriptCore/parser/Lexer.cpp:253
> +	   return "Invalid character: '\\#'";
> +    case 64:
> +	   return "Invalid character: '\\@'";
> +    case 96:
> +	   return "Invalid character: '\\`'";

These characters don't need to be escaped, so shouldn't be :-(

> Source/JavaScriptCore/parser/Lexer.cpp:1108
> +	       m_lexErrorMessage = "Only digits may follow a decimal point";

I think this would be better as "At least one digit must occur after a decimal
point"

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

Why have a string here at all in the clean state?

> Source/JavaScriptCore/parser/Lexer.h:78
> +	       m_lexErrorMessage = "Lexer error";

Default error message should be empty.	By doing this assignment I believe
you're adding at least two memory allocations and a memcpy to something we
expect to be very fast.

> Source/JavaScriptCore/parser/Parser.cpp:54
> +    UString lexErrorMessage = lexError ? lexer.getErrorMessage() :
UString();

I would do
lexErrorMessage = lexer.getErrorMessage();
ASSERT(lexErrorMessage.isNull() != lexError); // If there's been a lexer error
there should be a message, if there hasn't been, there should not be a message.