Boost logo

Boost :

From: Gavin Lambert (boost_at_[hidden])
Date: 2020-09-22 08:12:04

- Do you think the library should be accepted as a Boost library?

Yes, my vote is to ACCEPT this library.

- What is your evaluation of the design?

Much has been debated recently on the merits of push parsers vs. pull
parsers. Boost.JSON provides both a push parser (SAX-like interface)
and a DOM parser (which is somewhat pull-like but I don't think is
technically either kind).

It does not provide a "real" pull parser, which are sometimes more
efficient for deserialisation into custom types; the SAX parser can
somewhat do that but is not composable, which makes it harder to use in
practice, unless I'm missing something (this is usually why I end up
never using SAX parsers). If so, I'd like to see a more complete
example where the handler delegates part of the parsing to a class that
in turn delegates part to another class (as with reading typed
sub-values). I don't think this is currently possible.

So I think I can understand some of the debate around this point; while
I don't think it's strictly necessary that this particular library
implement that, if it doesn't then another library will end up having
potentially competing types and the natural Boost.Json name will already
be taken. Personally, however, I don't think this is sufficient reason
to block acceptance of this library.

Direct pull parsing doesn't mesh well with JSON, due to the requirement
to tolerate reordered keys, so any non-defective parser would have to
have something DOM-like under the hood anyway. (Some kind of cache, if
not necessarily a traditional DOM.) As such it should be sufficient to
use json::value itself as part of a pull-like parsing interface.

Regarding the SAX parser specifically, I dislike the *_part methods in
the callback interface. I feel that these should be removed and only
the final form retained, called only when the complete text of the value
has been determined. I don't feel it is ever sensible to call back with
a partially-parsed value (unless I am misunderstanding what these are
for, but there is little explanation in the docs, and I wasn't able to
get them called at all in my test).

I also don't see the point of supplying a size argument to on_key* and
on_string, as this can be read from the string_view argument instead.
The various number methods don't pass this separately. Similarly,
having a bool return as well as an error_code output argument seems
peculiar; one or the other should be sufficient, as is it just invites

parser.write() is incremental but basic_parser.write() is not, which
seems odd. I assume this means that parser is storing an incrementally
populated buffer of the full document rather than _really_ incrementally

parse/parser do not appear to support consuming only part of the
supplied input as a complete JSON value (returning how much was
consumed), which is useful for streams where multiple objects are
concatenated without explicit separation.

Regarding other aspects of design, I haven't drilled down in great
detail but the basic interface seems reasonably sound, other than the
concerns raised above.

I have mixed feelings on the initializer-list construction method -- I
like that there is some method of doing this, and it seems like
idiomatic C++ (particularly regarding how key-value pairs appear), but
also very non-JSON -- an array of objects ends up having three nested
braces. But I don't have any specific suggestion for improvement.

- What is your evaluation of the implementation?

It seems unfortunate (though understandable, due to heap allocation)
that you cannot declare a json::value / json::object as constexpr, for
cases where you do want some kind of constant reference object (e.g. a
schema description). You can somewhat work around this by declaring the
JSON as a constexpr string_view and later parsing it to a non-constexpr
json::value, but that seems a bit awkward (and potentially
bad-perf-prone if someone parses it multiple times).

Otherwise I didn't really look into the implementation at all.

- What is your evaluation of the documentation?

Overall, good, if perhaps a bit sparse on some aspects.

One pet peeve however is that most of the examples appear to be written
as if "using namespace boost::json" is in effect, which I feel both
encourages bad practice and makes it harder to distinguish which types
are provided by the library vs. exist only for the example. This is
especially confusing in the examples for tag_invoke, but not only there
-- another example of confusion are the aliases for types present in
both boost and std namespaces. I strongly recommend going through and
changing these to explicitly use a json:: namespace prefix on all
library types (including the boost/std aliases). Brevity is useful, but
not at the expense of clarity.

The "Using Numbers" section of the docs appears to be entirely blank.

"serializer can be used to incrementally serialize a value
incrementally" (redundant words are redundant)

(On a related note, an example use case for serializer is listed as
"ensure a fixed amount of work is performed in each cycle", but the only
means of control is the length of the provided buffer. I guess this is
equating work to number of characters output, which seems dubious. But
I have no specific objections.)

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

The library seems well suited for JSON as inter-process/network data
interchange, other than perhaps the omission of partial input parsing.

It seems less suited for JSON as configuration file -- while it can
parse such files (via common extensions such as ignoring comments and
trailing commas) it lacks means to pretty-print and to round-trip the
comments, which would be essential if writing back a config file
intended for human use. However while supporting this use case would be
nice, I don't consider it mandatory.

- Did you try to use the library? With what compiler? Did you have any

I tried it out with MSVC 2017 in standalone header-only mode, though
only with a few small examples.

The basic_parser examples do not compile out of the box -- with the
suggested json.hpp and src.hpp includes there are link errors with
basic_parser symbols. Additionally including basic_parser.hpp resolves
these but starts complaining about missing
max_{key,string,array,object}_size members of the handler type, which
are not documented. After these are supplied then it works as expected.
  I don't see why these should be needed, however.

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

Roughly four hours over a few days reviewing docs and trying some small

- Are you knowledgeable about the problem domain?

I have some apps that use JSON as both interchange and configuration
storage formats, although most of these do not use C++. So, "somewhat".

Boost list run by bdawes at, gregod at, cpdaniel at, john at