Basic support for reading and writing JSON objects.

Basic support for reading and writing JSON objects.

This is a minimal implementation that we can grow as needed
for debugger protocols and isolate communication.
Committed: https://code.google.com/p/dart/source/detail?r=6362

[turnidge, 2012-04-09 22:26:42]

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc
File runtime/vm/json.cc (right):

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode11
runtime/vm/json.cc:11: namespace dart {
The classes in the header go "scanner, reader, writer" and the classes in this
file go "writer, reader, scanner."  This is a small thing , but if there isn't a
pressing reason for them to be different, it would be nice to have them
consistent.  It helps me when I'm bouncing between header and soure.  Your call.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode22
runtime/vm/json.cc:22: if (buf_ != NULL) {
You don't need this if here.  It is safe to free/delete NULL.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode23
runtime/vm/json.cc:23: delete buf_;
Since you're using malloc above, should this be free()?  Either that or switch
the above to new[].

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode42
runtime/vm/json.cc:42: GrowBuffer(len + 64);  // Somewhat arbitrary slack.
Make the slack amount a named constant.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode59
runtime/vm/json.cc:59: char* new_buf =
reinterpret_cast<char*>(malloc(new_size));
Consider realloc?  It copies the bytes for you.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode60
runtime/vm/json.cc:60: intptr_t msg_len = OS::SNPrint(new_buf, new_size, "%s",
buf_);
I don't think you should use SNPrint to go the buffer copy here.  I don't know
if % is a legal character in json strings, but perhaps it is.  If so, printf
will try to interpret the %'s in the text and things will go astray.

If you use realloc, this becomes a non-issue.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode80
runtime/vm/json.cc:80: scanner_.Set(json_object_);
Why the call to Set here?  Isn't this done already by the constructor?

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h
File runtime/vm/json.h (right):

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode14
runtime/vm/json.h:14: // A low level interface to tokenize JSON strings.
Will anybody ever use this directly, or will they always use JSONReader instead?

It would be nice to remove this from the header entirely, but then it would be
hard to write direct scanner tests.  You could move this to the bottom of the
header file with a comment saying that it is in the header only so the unit test
can access it.  Your call.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode32
runtime/vm/json.h:32: explicit JSONScanner(const char* msg);
For consistency, consider renaming msg -> json_object?  Or perhaps json_text.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode34
runtime/vm/json.h:34: void Set(const char* str);
str -> json_object

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode55
runtime/vm/json.h:55: const char* string_;
string_ => json_object_

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode60
runtime/vm/json.h:60: // name-value pairs in JSON objects.
 JSONReader reparses the whole json text on every call to Seek().  This would be
good if we expected to only look up one value.  But I think that we will
probably care about all of the name-value pairs in a given command.

What do you think of returning a parse tree for the json object?

Right now, usage looks like this:

  reader.Set(rawText);
  if (reader.Seek("foo")) {
    reader.Set(reader.ValueChars());
    if (reader.Seek("subfield")) {
      // handle foo/subfield.
      ....
    }
  }
  if (reader.Error()) {
     ...error stuff...
  }
  // What if I forget to call Set here?
  reader.Set(rawText)
  if (reader.Seek("bar")) {
    ....other stuff
  }
  if (reader.Error()) {
     ...
  }

It's sort of awkward to use because you need to remember to call Set again every
time you pop back out to the outer level.  You also need to check for Error()
over and over again (I've left some of this out) because you may or may not
encounter the error on each call to Seek().  It is also awkward to iterate over
array elements in order.

If we just parsed the whole thing at once, we could check for errors once.  I
guess that would look something like this:

JSONNode* top = reader.Parse();
if (top.Error()) {
}
JSONNode* foo_node = top.Child("foo");
if (foo_node) {
  JSONNode* subfield = foo_node.Child("subfield");
  if (subfield) {
     ....
  }
}
...

Thoughts?

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode73
runtime/vm/json.h:73: void Set(const char* json_object);
Set is a pretty generic name.  Consider something more explanatory like
"SetText" or something like that.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode75
runtime/vm/json.h:75: // Returns true if the a pair with the given name was
found.
typo: "the a"

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode98
runtime/vm/json.h:98: 
Add a class-level comment here?

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode99
runtime/vm/json.h:99: class JSONWriter : ValueObject {
It's confusing to call this a JSONWriter since it doesn't have any json specific
functionality.  I would prefer giving this a more generic name and putting this
in its own file.  Maybe "PrintBuffer" or "CharBuffer" or something.

If it did have json-specific functionality, then the name would be less
confusing.  A JSONWriter would have functions like "WritePair", etc.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json_test.cc
File runtime/vm/json_test.cc (right):

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json_test.cc#newc...
runtime/vm/json_test.cc:12: TEST_CASE(JSON_ParseJSON) {
Change the test name to JSON_ScanJSON?  I think of parsing as being what the
JSONWriter does...

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json_test.cc#newc...
runtime/vm/json_test.cc:95: 
Add a test which detects a syntax error.

[hausner, 2012-04-10 16:26:51]

Thanks for the review.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc
File runtime/vm/json.cc (right):

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode11
runtime/vm/json.cc:11: namespace dart {
I typically don't read source in a linear way, so I didn't notice the
discrepancy.
Order changed to match the header file.

On 2012/04/09 22:26:42, turnidge wrote:
> The classes in the header go "scanner, reader, writer" and the classes in this
> file go "writer, reader, scanner."  This is a small thing , but if there isn't
a
> pressing reason for them to be different, it would be nice to have them
> consistent.  It helps me when I'm bouncing between header and soure.  Your
call.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode22
runtime/vm/json.cc:22: if (buf_ != NULL) {
On 2012/04/09 22:26:42, turnidge wrote:
> You don't need this if here.  It is safe to free/delete NULL.

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode23
runtime/vm/json.cc:23: delete buf_;
Nice catch, thank you.

On 2012/04/09 22:26:42, turnidge wrote:
> Since you're using malloc above, should this be free()?  Either that or switch
> the above to new[].

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode42
runtime/vm/json.cc:42: GrowBuffer(len + 64);  // Somewhat arbitrary slack.
On 2012/04/09 22:26:42, turnidge wrote:
> Make the slack amount a named constant.

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode59
runtime/vm/json.cc:59: char* new_buf =
reinterpret_cast<char*>(malloc(new_size));
On 2012/04/09 22:26:42, turnidge wrote:
> Consider realloc?  It copies the bytes for you.

Good suggestion, thank you.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode60
runtime/vm/json.cc:60: intptr_t msg_len = OS::SNPrint(new_buf, new_size, "%s",
buf_);
Very good point. Replaced with realloc().

On 2012/04/09 22:26:42, turnidge wrote:
> I don't think you should use SNPrint to go the buffer copy here.  I don't know
> if % is a legal character in json strings, but perhaps it is.  If so, printf
> will try to interpret the %'s in the text and things will go astray.
> 
> If you use realloc, this becomes a non-issue.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.cc#newcode80
runtime/vm/json.cc:80: scanner_.Set(json_object_);
On 2012/04/09 22:26:42, turnidge wrote:
> Why the call to Set here?  Isn't this done already by the constructor?

Because a JSON object is by definition an unordered set of pairs. The Seek()
interface does not assume any particular ordering of fields and thus starts from
the beginning of the object each time you look for a particular name.

An alternative interface (which we can add if desired) could iterate over the
fields in textual order.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h
File runtime/vm/json.h (right):

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode14
runtime/vm/json.h:14: // A low level interface to tokenize JSON strings.
The JSONReader declarations in this file need it. And since these are
ValueObject subtypes, not pointers, the order is dictated as well. (A forward
declaration doesn't work.)

On 2012/04/09 22:26:42, turnidge wrote:
> Will anybody ever use this directly, or will they always use JSONReader
instead?
> 
> It would be nice to remove this from the header entirely, but then it would be
> hard to write direct scanner tests.  You could move this to the bottom of the
> header file with a comment saying that it is in the header only so the unit
test
> can access it.  Your call.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode32
runtime/vm/json.h:32: explicit JSONScanner(const char* msg);
On 2012/04/09 22:26:42, turnidge wrote:
> For consistency, consider renaming msg -> json_object?  Or perhaps json_text.

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode34
runtime/vm/json.h:34: void Set(const char* str);
On 2012/04/09 22:26:42, turnidge wrote:
> str -> json_object

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode55
runtime/vm/json.h:55: const char* string_;
On 2012/04/09 22:26:42, turnidge wrote:
> string_ => json_object_
Actuall, string_ wasn't even used anywhere. Removed.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode60
runtime/vm/json.h:60: // name-value pairs in JSON objects.
As we discussed in person, this is a low-level interface where I didn't want to
allocate anything. The interface/class isn't complete, by all means. I expect
this to evolve according to our needs. If we find that for another use case, it
would make sense to create a value tree from the text buffer, we can add that as
an alternative interface, either within JSONReader or as a new class that uses
JSONScanner.


On 2012/04/09 22:26:42, turnidge wrote:
>  JSONReader reparses the whole json text on every call to Seek().  This would
be
> good if we expected to only look up one value.  But I think that we will
> probably care about all of the name-value pairs in a given command.
> 
> What do you think of returning a parse tree for the json object?
> 
> Right now, usage looks like this:
> 
>   reader.Set(rawText);
>   if (reader.Seek("foo")) {
>     reader.Set(reader.ValueChars());
>     if (reader.Seek("subfield")) {
>       // handle foo/subfield.
>       ....
>     }
>   }
>   if (reader.Error()) {
>      ...error stuff...
>   }
>   // What if I forget to call Set here?
>   reader.Set(rawText)
>   if (reader.Seek("bar")) {
>     ....other stuff
>   }
>   if (reader.Error()) {
>      ...
>   }
> 
> It's sort of awkward to use because you need to remember to call Set again
every
> time you pop back out to the outer level.  You also need to check for Error()
> over and over again (I've left some of this out) because you may or may not
> encounter the error on each call to Seek().  It is also awkward to iterate
over
> array elements in order.
> 
> If we just parsed the whole thing at once, we could check for errors once.  I
> guess that would look something like this:
> 
> JSONNode* top = reader.Parse();
> if (top.Error()) {
> }
> JSONNode* foo_node = top.Child("foo");
> if (foo_node) {
>   JSONNode* subfield = foo_node.Child("subfield");
>   if (subfield) {
>      ....
>   }
> }
> ...
> 
> Thoughts?

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode73
runtime/vm/json.h:73: void Set(const char* json_object);
On 2012/04/09 22:26:42, turnidge wrote:
> Set is a pretty generic name.  Consider something more explanatory like
> "SetText" or something like that.

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode75
runtime/vm/json.h:75: // Returns true if the a pair with the given name was
found.
On 2012/04/09 22:26:42, turnidge wrote:
> typo: "the a"

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode98
runtime/vm/json.h:98: 
On 2012/04/09 22:26:42, turnidge wrote:
> Add a class-level comment here?

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json.h#newcode99
runtime/vm/json.h:99: class JSONWriter : ValueObject {
On 2012/04/09 22:26:42, turnidge wrote:
> It's confusing to call this a JSONWriter since it doesn't have any json
specific
> functionality.  I would prefer giving this a more generic name and putting
this
> in its own file.  Maybe "PrintBuffer" or "CharBuffer" or something.
> 
> If it did have json-specific functionality, then the name would be less
> confusing.  A JSONWriter would have functions like "WritePair", etc.

I agree, this has nothing to do with JSON at this point. It's a formatter with
built-in buffer management. Renamed.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json_test.cc
File runtime/vm/json_test.cc (right):

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json_test.cc#newc...
runtime/vm/json_test.cc:12: TEST_CASE(JSON_ParseJSON) {
On 2012/04/09 22:26:42, turnidge wrote:
> Change the test name to JSON_ScanJSON?  I think of parsing as being what the
> JSONWriter does...

Done.

http://codereview.chromium.org/9949054/diff/2002/runtime/vm/json_test.cc#newc...
runtime/vm/json_test.cc:95: 
On 2012/04/09 22:26:42, turnidge wrote:
> Add a test which detects a syntax error.

Done.

[turnidge, 2012-04-11 17:06:45]

lgtm with 2 comments.

http://codereview.chromium.org/9949054/diff/7001/runtime/vm/json.cc
File runtime/vm/json.cc (right):

http://codereview.chromium.org/9949054/diff/7001/runtime/vm/json.cc#newcode45
runtime/vm/json.cc:45: while ((literal[i] != '\0') && (current_pos_[i] ==
literal[i])) {
What if current_pos_[i] == '\0'?  Might access beyond the end of the string. 
Please add a test for this case.

http://codereview.chromium.org/9949054/diff/7001/runtime/vm/json.cc#newcode61
runtime/vm/json.cc:61: while ((i < token_length_) && (token_start_[i] ==
literal[i])) {
What if literal is shorter than the current token?  You might access beyond the
end of the string.  Please add a test for this case.