[review][json] Boost.JSON review
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.
On Tue, Sep 22, 2020 at 2:59 AM Ruben Perez via Boost <boost@lists.boost.org> 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
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?
On Wed, Sep 23, 2020 at 7:55 AM Peter Dimov via Boost <boost@lists.boost.org> 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.
participants (3)
-
Peter Dimov
-
Ruben Perez
-
Vinnie Falco