[webkit-reviews] review denied: [Bug 63340] Lexer error messages are not very informative : [Attachment 99397] revised patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Jun 30 18:09:18 PDT 2011
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.
More information about the webkit-reviews
mailing list