Add error messages to JSONReader and friends. This required a bit of refactor...

Add error messages to JSONReader and friends. This required a bit of refactoring to do cleanly. This CL doesn't actually use this capability anywhere except for unit tests. I will add that in a future CL.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=6459

[tony, 2008-12-05 18:17:24]

http://codereview.chromium.org/13169/diff/201/202
File base/json_reader.cc (right):

http://codereview.chromium.org/13169/diff/201/202#newcode154
Line 154: if (error_message_.size() == 0)
Nit: empty()

http://codereview.chromium.org/13169/diff/201/203
File base/json_reader.h (right):

http://codereview.chromium.org/13169/diff/201/203#newcode94
Line 94: std::string *error_message_out);
Alternately, if you don't want to change all the callers, you could add a
separate method ReadWithError that does this.  Read would call ReadWithError and
just ignore the error message.

http://codereview.chromium.org/13169/diff/201/203#newcode109
Line 109: bool allow_trailing_comma, std::string* error_message_out);
Would it be easier to not add this extra param and just add a error_message()
method that people who want the error message can call?  Then you don't have to
null check the string in the code.

http://codereview.chromium.org/13169/diff/201/203#newcode168
Line 168: // Pointer to a string for errors message encountered during parsing.
Nit: This isn't a pointer.

[Aaron Boodman, 2008-12-05 20:23:27]

Ready for another look.

http://codereview.chromium.org/13169/diff/201/202
File base/json_reader.cc (right):

http://codereview.chromium.org/13169/diff/201/202#newcode154
Line 154: if (error_message_.size() == 0)
On 2008/12/05 18:17:24, tony wrote:
> Nit: empty()

Done.

http://codereview.chromium.org/13169/diff/201/203
File base/json_reader.h (right):

http://codereview.chromium.org/13169/diff/201/203#newcode94
Line 94: std::string *error_message_out);
On 2008/12/05 18:17:24, tony wrote:
> Alternately, if you don't want to change all the callers, you could add a
> separate method ReadWithError that does this.  Read would call ReadWithError
and
> just ignore the error message.

Done, but I still had to change all the callers to the JSONStringSerializer and
JSONFileSerializer classes. I think that some sort of composable design for
these classes might be better, added a TODO about that to the top.

http://codereview.chromium.org/13169/diff/201/203#newcode109
Line 109: bool allow_trailing_comma, std::string* error_message_out);
On 2008/12/05 18:17:24, tony wrote:
> Would it be easier to not add this extra param and just add a error_message()
> method that people who want the error message can call?  Then you don't have
to
> null check the string in the code.

Yes, good idea. Done.

http://codereview.chromium.org/13169/diff/201/203#newcode168
Line 168: // Pointer to a string for errors message encountered during parsing.
On 2008/12/05 18:17:24, tony wrote:
> Nit: This isn't a pointer.

Done.

[tony, 2008-12-05 20:37:57]

I think you could add a GetErrorMessage method to JSONSerializer so you don't
have to change the callers.  Either way is fine with me.

LGTM.

http://codereview.chromium.org/13169/diff/41/405
File base/json_reader.h (right):

http://codereview.chromium.org/13169/diff/41/405#newcode117
Line 117: std::string* error_message() { return &error_message_; }
Nit: Can we just return a string here?  I think the extra copy is ok and then
the string won't deref unexpectedly when JSONReader goes out of scope.