Bug 26587 - Support JSON.parse
: Support JSON.parse
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
: 26591
: 20031 26325 26503
  Show dependency treegraph
 
Reported: 2009-06-21 12:51 PST by
Modified: 2009-06-22 21:30 PST (History)


Attachments
Initial JSON.parse support (24.83 KB, patch)
2009-06-21 13:03 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff
JSON.parse, this time correct (46.40 KB, patch)
2009-06-21 15:41 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff
Address rest of darin's comments (46.28 KB, patch)
2009-06-21 15:53 PST, Oliver Hunt
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-06-21 12:51:06 PST
We have a JSON object but no JSON.parse
------- Comment #1 From 2009-06-21 13:03:00 PST -------
Created an attachment (id=31617) [details]
Initial JSON.parse support
------- Comment #2 From 2009-06-21 13:16:10 PST -------
(From update of attachment 31617 [details])
> -    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.
------- Comment #3 From 2009-06-21 13:23:10 PST -------
(From update of attachment 31617 [details])
removing review flag as there appears to be an error in the parser now
------- Comment #4 From 2009-06-21 15:41:10 PST -------
Created an attachment (id=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
------- Comment #5 From 2009-06-21 15:53:33 PST -------
Created an attachment (id=31623) [details]
Address rest of darin's comments
------- Comment #6 From 2009-06-21 15:57:21 PST -------
(From update of attachment 31623 [details])
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.
------- Comment #7 From 2009-06-21 16:08:54 PST -------
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
------- Comment #8 From 2009-06-21 16:32:53 PST -------
Build fix
Committing to http://svn.webkit.org/repository/webkit/trunk ...
    M    JavaScriptCore/ChangeLog
    M    JavaScriptCore/runtime/LiteralParser.cpp
Committed r44924