Bug 26587

Created attachment 31617 [details] Initial JSON.parse support

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 on attachment 31617 [details] Initial JSON.parse support removing review flag as there appears to be an error in the parser now

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

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

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.

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

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