[webkit-reviews] review denied: [Bug 63339] JSON errors should be informative : [Attachment 101695] Work in progress

[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 63339: JSON errors should be informative
https://bugs.webkit.org/show_bug.cgi?id=63339

Attachment 101695: Work in progress
https://bugs.webkit.org/attachment.cgi?id=101695&action=review

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


r-, due to some code style issues and error message clarity.   Would be good to
also store the value of m_ptr for error feedback, then we can give line and
column information.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:240
> +		   m_lexErrorMessage = "JSON Lexer error: Single quotes are not
allowed in JSON";

Call out the specific character as well just to make it clear

> Source/JavaScriptCore/runtime/LiteralParser.cpp:281
> +		   m_lexErrorMessage = "JSON Lexer error: Unexpected end of
string";

Unterminated string

> Source/JavaScriptCore/runtime/LiteralParser.cpp:325
> +			       m_lexErrorMessage = "JSON Lexer error: Escaped
unicode is not a ASCII hexadecimal digit";

I would say "\\u..." is not a valid unicode escape

> Source/JavaScriptCore/runtime/LiteralParser.cpp:436
> +	       m_lexErrorMessage = "JSON Lexer error: Exponent symbols should
be followed by '+' or '-' and least one number";

+/- are optional

> Source/JavaScriptCore/runtime/LiteralParser.cpp:608
> +			   m_parseErrorMessage = "JSON Parse error: Could not
parse value expression";

At this point you either have a bogus token -- so the lexer should have an
error message, or you should be able to say what token you got and why it is
wrong.

> Source/JavaScriptCore/runtime/LiteralParser.cpp:638
> +		   m_parseErrorMessage = "JSON Parse error: Expected end of
JSON but file continued";

Unexpected content at end of JSON literal.

> Source/JavaScriptCore/runtime/LiteralParser.h:97
> +	       UString m_lexErrorMessage;

make it private

> Source/JavaScriptCore/runtime/LiteralParser.h:112
> +		   : m_lexErrorMessage()

UString has a default constructor so this isn't necessary