[webkit-reviews] review granted: [Bug 26587] Support JSON.parse : [Attachment 31617] Initial JSON.parse support

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Jun 21 13:16:10 PDT 2009


Darin Adler <darin at apple.com> has granted Oliver Hunt <oliver at apple.com>'s
request for review:
Bug 26587: Support JSON.parse
https://bugs.webkit.org/show_bug.cgi?id=26587

Attachment 31617: Initial JSON.parse support
https://bugs.webkit.org/attachment.cgi?id=31617&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    LiteralParser preparser(callFrame, programSource);
> +    LiteralParser preparser(callFrame, programSource, false);

I am not a fan of booleans for this sort of thing. A named enum would work
better here.

> +    LiteralParser preparser(exec, source, true);

I think preparser is a strange name here. How about just "parser".

> +    if (JSValue parsedObject = preparser.tryJSONParse())
> +	   return parsedObject;
> +    
> +    return throwError(exec, SyntaxError, "Unable to parse JSON string");

I would have written this the other way around, with the early return for the
error case. You couldn't scope the return value, but otherwise it would be
better.

> +		   token.end = m_ptr += 4;

Merging these two statements into one line seems too clever and unnecessarily
subtle and hence hard to read. Just m_ptr += 4 on one line and token.end =
m_ptr on another should be fine.

> +	   case 'f': {

No need for this extra set of braces on all these cases.

> +static bool isSafeStringCharacter(UChar c)
> +{
> +    return (c >= ' ' && c <= 0xff && c != '\\' && c != '"') || c == '\t';
> +}

Could I suggest that you use inline here?

Also, I think the ? : operator would be better. If it's >= ' ' you can do one
set of checks, and if it's < ' ' you can do the '\t' check.

    return c >= ' ' ? c <= 0xFF && c != '\\' && c != '"' : c == '\t';

> +		       if ((m_end - m_ptr) < 5) // uNNNN == 5 characters

Extra parentheses here seem unneeded.

> +		      
token.stringToken.append(JSC::Lexer::convertUnicode(m_ptr[1], m_ptr[2],
m_ptr[3], m_ptr[4]));

Does this do the right thing for illegal UTF-16?

> +		       case TokNull: {

> +		       case TokTrue: {

> +		       case TokFalse: {

These extra braces here are unneeded.

> +	   JSValue tryJSONParse()

I think it's strange that this parser has both a strict boolean in the
constructor *and* a separate entry point for the JSON parse. Is that really
necessary?

> +	   {
> +	       m_lexer.next();
> +	       JSValue result = parse(StartParseExpression);
> +	       if (m_lexer.currentToken().type != TokEnd)
> +		   return JSValue();
> +	       return result;
> +	   }

Does this really need to be inline? Can't some of the code just be defined
normally without putting it all in the header?

Patch has test, but is missing the test results.

r=me as long as you deal with the test result issue, but please consider some
of the other comments too.


More information about the webkit-reviews mailing list