text: We have a JSON object but no JSON.parse
  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 ...
  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 ...
  M JavaScriptCore/ChangeLog
  M JavaScriptCore/runtime/LiteralParser.cpp
  Committed r44924