[review][json] Boost.JSON review

From: Ruben Perez (rubenperez038_at_[hidden])
Date: 2020-09-22 09:59:06

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.