Bug 26587 - Support JSON.parse
: Support JSON.parse
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore
: 528+ (Nightly build)
: Macintosh Mac OS X 10.5
: P2 Normal
Assigned To: Oliver Hunt
:
Depends on: 26591
Blocks: 20031 26325 26503
  Show dependency treegraph
 
Reported: 2009-06-21 12:51 PDT by Oliver Hunt
Modified: 2009-06-22 21:30 PDT (History)
0 users

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2009-06-21 12:51:06 PDT
We have a JSON object but no JSON.parse
Comment 1 Oliver Hunt 2009-06-21 13:03:00 PDT
Created attachment 31617 [details]
Initial JSON.parse support
Comment 2 Darin Adler 2009-06-21 13:16:10 PDT
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.
Comment 3 Oliver Hunt 2009-06-21 13:23:10 PDT
Comment on attachment 31617 [details]
Initial JSON.parse support

removing review flag as there appears to be an error in the parser now
Comment 4 Oliver Hunt 2009-06-21 15:41:10 PDT
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
Comment 5 Oliver Hunt 2009-06-21 15:53:33 PDT
Created attachment 31623 [details]
Address rest of darin's comments
Comment 6 Cameron Zwarich (cpst) 2009-06-21 15:57:21 PDT
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.
Comment 7 Oliver Hunt 2009-06-21 16:08:54 PDT
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 Oliver Hunt 2009-06-21 16:32:53 PDT
Build fix
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	JavaScriptCore/ChangeLog
	M	JavaScriptCore/runtime/LiteralParser.cpp
Committed r44924