Issue 18168003: JSONStream and tests

Issue 18168003: JSONStream and tests (Closed)

Created:
7 years, 5 months ago by Cutch

Modified:
7 years, 5 months ago

Reviewers:
Ivan Posva

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

JSONStream and tests R=iposva@google.com Committed: https://code.google.com/p/dart/source/detail?r=24697

Patch Set 6 : #

Created: 7 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+286 lines, -0 lines)			Patch
A	runtime/vm/json_stream.h	View	1 2 3 4	1 chunk	+46 lines, -0 lines	0 comments	Download
A	runtime/vm/json_stream.cc	View	1 2 3 4	1 chunk	+135 lines, -0 lines	0 comments	Download
M	runtime/vm/json_test.cc	View	1 2 3 4 5	2 chunks	+103 lines, -0 lines	0 comments	Download
M	runtime/vm/vm_sources.gypi	View	1 4	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 6 (0 generated)

Expand Messages | Collapse Messages

Ivan Posva

Patchset #3 LGTM with some comments. We should plan to iterate on this as it ...

7 years, 5 months ago (2013-07-01 23:18:14 UTC) #3

Cutch

https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc File runtime/vm/json_stream.cc (right): https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc#newcode14 runtime/vm/json_stream.cc:14: open_objects_ = 0; On 2013/07/01 23:18:14, Ivan Posva wrote: ...

7 years, 5 months ago (2013-07-01 23:33:55 UTC) #4

Cutch

On 2013/07/01 23:33:55, Cutch wrote: > https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc > File runtime/vm/json_stream.cc (right): > > https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc#newcode14 > ...

7 years, 5 months ago (2013-07-02 15:43:17 UTC) #5

On 2013/07/01 23:33:55, Cutch wrote:
> https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc
> File runtime/vm/json_stream.cc (right):
> 
>
https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc#...
> runtime/vm/json_stream.cc:14: open_objects_ = 0;
> On 2013/07/01 23:18:14, Ivan Posva wrote:
> > What is open_object_ used for?
> 
> Sanity check for calls to OpenObject / CloseObject.
> 
>
https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc#...
> runtime/vm/json_stream.cc:18: JSONStream::~JSONStream() {
> On 2013/07/01 23:18:14, Ivan Posva wrote:
> > Who is charged with freeing the TextBuffer?
> 
> TextBuffer is a ValueObject so it is cleaned up when the stack unwinds.
> 
>
https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc#...
> runtime/vm/json_stream.cc:26: 
> On 2013/07/01 23:18:14, Ivan Posva wrote:
> > 2 lines.
> 
> Done.
> 
>
https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.cc#...
> runtime/vm/json_stream.cc:55: buffer_->Printf("%s", b ? "true" : "false");
> On 2013/07/01 23:18:14, Ivan Posva wrote:
> > How about adding a private PrintBool(bool b) and call this from here and
> > PrintPropertyBool? This would make the decision as to when to check for
> > NeedsComma easier.
> 
> Not sure what this has to do with PrintCommaIfNeeded, it needs to be called
> before each value is printed.
> 
> https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.h
> File runtime/vm/json_stream.h (right):
> 
>
https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.h#n...
> runtime/vm/json_stream.h:18: void Clear();
> On 2013/07/01 23:18:14, Ivan Posva wrote:
> > When do you expect to ever have to call Clear() on a currently existing
> > JSONStream?
> 
> Possibly never. I can remove this.
> 
>
https://codereview.chromium.org/18168003/diff/5001/runtime/vm/json_stream.h#n...
> runtime/vm/json_stream.h:36: void PrintPropertyName(const char* name);
> On 2013/07/01 23:18:14, Ivan Posva wrote:
> > Shouldn't this be a private?
> 
> Not without a change to OpenObject.  This is needed for outputting child
> objects, for example:
> 
> OpenObject(); // outputs {
> PrintPropertyName("childObject"); // outputs "childObject":
> OpenObject(); // outputs {
> CloseObject(); // outputs }
> CloseObject(); // outputs }
> 
> I can make this private and modify OpenObject to take the (optional) name of
the
> child object.


I have made PrintPropertyName private and added optional name fields to
OpenObject and OpenArray.

Cutch

7 years, 5 months ago (2013-07-02 16:58:07 UTC) #6

Message was sent while issue was closed.

Committed patchset #6 manually as r24697 (presubmit successful).

Expand Messages | Collapse Messages