jsoncpp-devel

Re: [Jsoncpp-devel] jsoncpp patches from Google

[Baptiste L. <bap...@gm...>]

2011/5/23 Aaron Jacobs <ja...@go...>

> Hi Baptiste and jsoncpp-devel,
>
> I'm a software engineer at Google, where we use jsoncpp for a few projects.
> We've made a number of modifications internally, and I'd like to contribute
> them back to the public repository.
>
>
Thanks for contributing this back.

Below are some general comments concerning the individual patches.


> Here are a few patches that are ready to apply to SVN revision 192:
>
>    https://github.com/jacobsa/jsoncpp/commit/d7377e3c438490

=> OK

To make sure I understand the issue: sscanf on Mac OS X will write in the
format string provided as parameter even though it is declared const char *?

    https://github.com/jacobsa/jsoncpp/commit/7d2314b1fef8f7
=> OK

   https://github.com/jacobsa/jsoncpp/commit/1de4ede76d27b0

=> I see a few problems with this patch:
- JSON_FAIL_MESSAGE is bugged if there is no exception (e.g.
multi-statements). IMHO it should just call a function that dump message and
then call abort().
- partial duplication of assertions macros already present in json_value.cpp

I think we should move the assertion/failure macros from json_value.cpp  to
a common private header instead of duplicating them.

In the future, we may want to provide hook to allow user to customize what
happens on failure (e.g. like google breakpad integration or whatever stack
capture tool you happen to use...)

   https://github.com/jacobsa/jsoncpp/commit/8945bfb0a29235

=> OK

What's the easiest way to get these committed to the main jsoncpp SVN repo?
> I've got several more changes to contribute back, but wanted to get the
> ball
> rolling with these first.
>

I'll contact you privately.

Thanks for your time,
Baptiste.


> Thanks,
> Aaron Jacobs
>

Re: [Jsoncpp-devel] jsoncpp patches from Google

[Aaron J. <ja...@go...>]

On Tue, May 24, 2011 at 3:06 AM, Baptiste Lepilleur
<bap...@gm...> wrote:
> To make sure I understand the issue: sscanf on Mac OS X will write in the
> format string provided as parameter even though it is declared const char *?

That's right. Don't ask me. :-/

> - JSON_FAIL_MESSAGE is bugged if there is no exception (e.g.
> multi-statements). IMHO it should just call a function that dump message and
> then call abort().

Sorry, I'm not sure I understand the first sentence here. Could you clarify?


> - partial duplication of assertions macros already present in json_value.cpp
>
> I think we should move the assertion/failure macros from json_value.cpp  to
> a common private header instead of duplicating them.
> In the future, we may want to provide hook to allow user to customize what
> happens on failure (e.g. like google breakpad integration or whatever stack
> capture tool you happen to use...)

Okay, sounds good.

Re: [Jsoncpp-devel] jsoncpp patches from Google

[Baptiste L. <bap...@gm...>]

2011/5/24 Aaron Jacobs <ja...@go...>

> On Tue, May 24, 2011 at 3:06 AM, Baptiste Lepilleur
> <bap...@gm...> wrote:
> [...]
> > - JSON_FAIL_MESSAGE is bugged if there is no exception (e.g.
> > multi-statements). IMHO it should just call a function that dump message
> and
> > then call abort().
>
> Sorry, I'm not sure I understand the first sentence here. Could you
> clarify?
>

Sorry, I should have made this clearer:
#define JSON_FAIL_MESSAGE( message ) std::cerr << message; exit(123);
with:
if (!ok) JSON_FAIL_MESSAGE(reader.getFormattedErrorMessages());

will becomes:
if (!ok)
std::cerr << message;
exit(123);

A simple way to avoid this is:
#define JSON_FAIL_MESSAGE( message ) onJsonFailure( message );

Baptiste.

Re: [Jsoncpp-devel] jsoncpp patches from Google

[Aaron J. <ja...@go...>]

Ah right, I see. I actually fixed this by adding braces in the SVN
commit I made, but if you want me to add a function I can.

On Wed, May 25, 2011 at 1:43 AM, Baptiste Lepilleur
<bap...@gm...> wrote:
>
>
> 2011/5/24 Aaron Jacobs <ja...@go...>
>>
>> On Tue, May 24, 2011 at 3:06 AM, Baptiste Lepilleur
>> <bap...@gm...> wrote:
>> [...]
>> > - JSON_FAIL_MESSAGE is bugged if there is no exception (e.g.
>> > multi-statements). IMHO it should just call a function that dump message
>> > and
>> > then call abort().
>>
>> Sorry, I'm not sure I understand the first sentence here. Could you
>> clarify?
>
> Sorry, I should have made this clearer:
> #define JSON_FAIL_MESSAGE( message ) std::cerr << message; exit(123);
> with:
> if (!ok) JSON_FAIL_MESSAGE(reader.getFormattedErrorMessages());
> will becomes:
> if (!ok)
> std::cerr << message;
> exit(123);
> A simple way to avoid this is:
> #define JSON_FAIL_MESSAGE( message ) onJsonFailure( message );
> Baptiste.
>