[review][json] My review

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 
inconsistency.

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 
parsing.

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 
problems?

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 
experiments.

- 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".