[review][json] Boost.JSON review

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[review][json] Boost.JSON review

Boost - Dev mailing list
Hi all,

My recommendation is to ACCEPT Boost.JSON into Boost. Parsing and
serializing
JSON using a DOM representation is a common enough task to have a
specialized
library for it in Boost. Most other languages provide built-in support
for this task. I see Boost as the repository where I look at when the
standard doesn't provide me something it should, so having a DOM
JSON library in Boost is a must for me.

- What is your evaluation of the design?

Generally sound. I especially value its simplicity. Concretely:
  1. The memory management model is extremely simple. I wonder if
     storage_ptr could be moved somewhere else long term so other
     libraries can use it.
  2. Using concrete types for numbers (uint64_t/int64_t/double)
     may not fit everyone's needs but will definitely fit most
     and makes things easy.

I miss some way of accessing json::value from generic code,
but I understand visitation is currently in progress. As a user
I would also be satisfied with generic versions of get_xxx,
is_xxx and so on:

```cpp
json::value val = /* ... */
val.get<json::object>() // or json::get<json::object>(val)
```

I also miss some way of determining the location (line and column)
of parse errors. I don't know how this feature would interact with
performance though. Nice to have but not required for me.

I would have also liked `json::kind` be streamable, as it
makes logging/debugging easier in some contexts. Nothing I can't
live without, though.

Finally, I would also appreciate `json::string` have a
straightforward way to be converted to `std::string` (e.g.
by having a `to_std_string()` member function).

- What is your evaluation of the implementation?

I have just looked at the interface.

- What is your evaluation of the documentation?

The documentation is very good and detailed, nice work.

I would say the main point of improvement would be `basic_parser`,
as I had a hard time understanding when some of the callbacks
get called, especially the `on_xxx_part` ones. It would be also good
to know if accessing the `basic_parser` state (e.g. `depth()`) is allowed
from within the handler. And if it is, maybe passing the parser instance
at the beginning of the parse could be useful (e.g. in the
`on_document_begin`,
or in a separate hook).

Ideally I would like to see an example of `basic_parser` used to parse
a custom struct, if it's not excessively complex.

Also note that `basic_parser` docs say that it's defined in
`<boost/json/detail/basic_parser.hpp>` and that the convenience
header is `<boost/json.hpp>`. Instead, it would be good to note
that the user should include `<boost/json/basic_parser.hpp>` and
that `<boost/json.hpp>` is not enough (I understand for build speed
reasons).

I've noted that streaming a `json::string` outputs the string
within double quotes. It surprised me at first, as I was working
with the 'json::string equals std::string' mentality. I guess this
behavior makes sense, but I would make a note in the 'Using strings'
section.

'See also' section for
https://master.json.cpp.al/json/ref/boost__json__value_from_tag.html
renders functions without separation, looks strange.

I would make a note in the Value conversion mechanism referencing
P1895 on `tag_invoke` so users who don't know this paper don't
stare at the name wondering 'why?'

As a final note, googling 'Boost json' renders this link to an old
version of the docs: http://vinniefalco.github.io/doc/json/index.html
Consider removing it so users don't get confused (I did at the very
beginning).

- What is your evaluation of the potential usefulness of the library?

I think it will be incredibly useful. Parsing to DOM and serializing from
DOM are very widespread tasks, and having a simple, well designed
`json::value` container makes things much better. I would have used
it for my use cases if it had been available.

- Did you try to use the library? With which compiler(s)? Did you have any
problems?

I used the library in the Boost version and separate compilation model.
I built and installed the library using CMake 3.16.3 and
g++ 9.3.0 on Ubuntu 20.04, with no problems. I then built several
toy examples with the library's CMake `find_package` interface.
No problems.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I spent around 10h reading the documentation and building examples.

- Are you knowledgeable about the problem domain?

I have used JSON as a data exchange format in the embedded, financial and
web worlds, in C++ and other languages (JavaScript, Python and PHP).
In C++ I have mainly used rapidJSON, and I would replace it with
Boost.JSON for future developments. I have no experience writing
JSON libraries myself.

Thanks,
Ruben.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [review][json] Boost.JSON review

Boost - Dev mailing list
On Tue, Sep 22, 2020 at 2:59 AM Ruben Perez via Boost
<[hidden email]> wrote:
> My recommendation is to ACCEPT Boost.JSON into Boost.

Thank you for looking at the library and writing up a review.

>   1. The memory management model is extremely simple. I wonder if
>      storage_ptr could be moved somewhere else long term so other
>      libraries can use it.

This is something that has come up a few times, and I think it is
worth the review manager making a special note of it. For now I am
content to simply have an initial release of Boost.JSON where the
storage_ptr system is part of the library (which will be necessary for
standalone anyway). And we can see how the community feels about it,
work out any possible kinks.

After some time for the thing to mature, we can measure support for
proposing it as its own library, or perhaps making it part of another
library.

> I miss some way of accessing json::value from generic code,
> but I understand visitation is currently in progress. As a user
> I would also be satisfied with generic versions of get_xxx,
> is_xxx and so on:

I'm open to it, but could you open an issue describing exactly how
this would work? What types are permissible to pass to `get<>`? Does
it have to be a member function (because you have to put the word
template there in generic contexts). This is worth exploring.

> I also miss some way of determining the location (line and column)
> of parse errors. I don't know how this feature would interact with
> performance though. Nice to have but not required for me.

Well, basic_parser is written so that the return value from write
(soon, write_some) is exactly the number of valid characters before a
parse error occurred. So you can find out precisely where in the
buffer the problem happened. It would be pretty easy to write your own
function which uses parser and also communicates this information
somehow (perhaps an out-param).

> I would have also liked `json::kind` be streamable, as it
> makes logging/debugging easier in some contexts. Nothing I can't
> live without, though.

We can do that. Track it here:

<https://github.com/CPPAlliance/json/issues/405>

> Finally, I would also appreciate `json::string` have a
> straightforward way to be converted to `std::string` (e.g.
> by having a `to_std_string()` member function).

hmmm... I'm not opposed to this in principle, but then people with no
need for this conversion would be including <string>. We are
unfortunately including that already, but I have plans to not include
it. Track it here:

<https://github.com/CPPAlliance/json/issues/406>

> I would say the main point of improvement would be `basic_parser`,
> as I had a hard time understanding when some of the callbacks
> get called, especially the `on_xxx_part` ones. It would be also good
> to know if accessing the `basic_parser` state (e.g. `depth()`) is allowed
> from within the handler. And if it is

> maybe passing the parser instance
> at the beginning of the parse could be useful (e.g. in the
> `on_document_begin`,

If we do this, the handler would receive `basic_parser<Handler>
const&` as the first argument on construction. Yes you can access
`depth()`. Track the issue here:

<https://github.com/CPPAlliance/json/issues/408>

> I've noted that streaming a `json::string` outputs the string
> within double quotes. It surprised me at first, as I was working
> with the 'json::string equals std::string' mentality. I guess this
> behavior makes sense, but I would make a note in the 'Using strings'
> section.

Yes, track that here:

<https://github.com/CPPAlliance/json/issues/409>

> 'See also' section for
> https://master.json.cpp.al/json/ref/boost__json__value_from_tag.html
> renders functions without separation, looks strange.

Typo :) Track here:

<https://github.com/CPPAlliance/json/issues/410>

> I would make a note in the Value conversion mechanism referencing
> P1895 on `tag_invoke` so users who don't know this paper don't
> stare at the name wondering 'why?'

Agree! Track:

<https://github.com/CPPAlliance/json/issues/411>

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [review][json] Boost.JSON review

Boost - Dev mailing list
Vinnie Falco wrote:

> >   1. The memory management model is extremely simple. I wonder if
> > storage_ptr could be moved somewhere else long term so other libraries
> > can use it.
>
> This is something that has come up a few times, and I think it is worth
> the review manager making a special note of it. For now I am content to
> simply have an initial release of Boost.JSON where the storage_ptr system
> is part of the library (which will be necessary for standalone anyway).
> And we can see how the community feels about it, work out any possible
> kinks.
>
> After some time for the thing to mature, we can measure support for
> proposing it as its own library, or perhaps making it part of another
> library.

How is this going to work in standalone mode?


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [review][json] Boost.JSON review

Boost - Dev mailing list
On Wed, Sep 23, 2020 at 7:55 AM Peter Dimov via Boost
<[hidden email]> wrote:
> > After some time for the thing to mature, we can measure support for
> > proposing it as its own library, or perhaps making it part of another
> > library.
>
> How is this going to work in standalone mode?

Yes that's a problem.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost