[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.getjson::object() // or json::getjson::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
`
On Tue, Sep 22, 2020 at 2:59 AM Ruben Perez via Boost
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?
participants (3)
-
Peter Dimov
-
Ruben Perez
-
Vinnie Falco