Bug 26587 – Support JSON.parse

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

We have a JSON object but no JSON.parse

[Oliver Hunt]

Created attachment 31617 [details]
Initial JSON.parse support

[Darin Adler]

Comment on attachment 31617 [details]
Initial JSON.parse support

> -    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.

[Oliver Hunt]

Comment on attachment 31617 [details]
Initial JSON.parse support

removing review flag as there appears to be an error in the parser now

[Oliver Hunt]

Created attachment 31622 [details]
JSON.parse, this time correct

This is identical to the previous patch except for a few minor changes.  I re-added the string token reset i had moronically removed prior to submission.  And added tests that catch the behaviour it introduced.

Restructured JSONProtoFuncParse to exit early on failure

[Oliver Hunt]

Created attachment 31623 [details]
Address rest of darin's comments

[Cameron Zwarich (cpst)]

Comment on attachment 31623 [details]
Address rest of darin's comments

You still have instances of

token.end = m_ptr += 5;

but you say you have fixed them locally. Now that you have removed the braces, you should probably decide on a consistent style for whether or not there are blank lines after the case statements inside of a switch. Other than that, r=me.

[Oliver Hunt]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/interpreter/Interpreter.cpp
	M	JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp
	M	JavaScriptCore/runtime/JSONObject.cpp
	M	JavaScriptCore/runtime/LiteralParser.cpp
	M	JavaScriptCore/runtime/LiteralParser.h
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/js/JSON-parse-expected.txt
	A	LayoutTests/fast/js/JSON-parse.html
	A	LayoutTests/fast/js/resources/JSON-parse.js
Committed r44923

[Oliver Hunt]

Build fix
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/LiteralParser.cpp
Committed r44924