On 14.09.20 09:30, Pranam Lashkari via Boost wrote:
Please provide in your review information you think is valuable to understand your choice to ACCEPT or REJECT including JSON as a Boost library. Please be explicit about your decision (ACCEPT or REJECT).
- What is your evaluation of the design?
Most of it seems fine, but I do have some issues: - The choice of [u]int64_t seems arbitrary and restrictive. It means that Boost.JSON will not use a 128 bit integer even where one is available, and it means that it cannot compile at all on implementations that don't provide int64_t. It's good enough for my purposes, but I would like to see some discussion about this. The json spec allows arbitrarily large integers. - On a similar note, the use of double restricts floating point accuracy even when a higher-precision type is available. The json spec allows arbitrarily precise decimal values. - boost::json::value_to provides a single, clean way to extract values from json, but it also renders other parts of the library (e.g. number_cast) redundant except as an implementation detail. - boost::json::value_to provides a single, clean way to extract values from json, but it's syntactically long. The same functionality in a member function of boost::json::value would be nicer to use. - The omission of binary serialization formats (CBOR et al) bothers me. Not from a theoretical point of view, but because I have actual code that uses CBOR, and I won't be able to convert this code to Boost.JSON unless CBOR support is provided. (Or I could write my own, but that rather defeats the point of using a library in the first place, especially if Neils Lohmann's library already provides CBOR support.)
- What is your evaluation of the implementation?
I didn't look at it.
- What is your evaluation of the documentation?
boost::json::value_to provides a single, clean way to extract values from json, but it's actually rather hard to find in the documentation. I was looking for a way to extract a std::string from a boost::json::value, so I looked at the documentation for boost::json::value and found as_string. OK, that returns boost::json::string, which is not implicitly convertible to std::string (in C++14). But it has an implicit conversion to string_view, which is an alias of boost::string_view, which doesn't appear to be documented anywhere but which (looking at the source code) has a member function to_string. So I ended up with this code: boost::json::value v = "Hello world."; std::string s = static_castboost::json::string_view(v.as_string()).to_string(); ...which I think we can all agree is an abomination. /Then/ I found out about boost::json::value_to, and replaced my code with this: std::string s = boost::json::value_tostd::string(v); Which is definitely nicer, though still not as nice as Niels Lohmann's code: std::string s = v.getstd::string(); boost::json::value_to or its member function replacement should definitely be front and center to the documentation.
- What is your evaluation of the potential usefulness of the library?
A good json library is very useful for a large number of programs. The questions isn't if Boost should have a json library, but if this is the json library that Boost should have.
- Did you try to use the library? With which compiler(s)? Did you have any problems?
I converted a small but non-trivial program from Lohmann's json to the proposed Boost.JSON, and compiled it with several different cross-compilers. I did not encounter any problems compiling or running the program, although I did have to add Boost.Container to the set of linked libraries.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A few hours of work, most of which was spent converting code from Lohmann's json library to the proposed Boost.JSON.
- Are you knowledgeable about the problem domain?
Yes.
Please be explicit about your decision (ACCEPT or REJECT).
For me, the ultimate question is if I would actually use this library, and my reluctant answer is "not its current state". I'm basically satisfied with Lohmann's json library, which requires less verbosity to use and which provides CBOR support. I can see the attraction of Boost.JSON's superior performance, and the attraction of incremental parsing and serialization, but for my usage none of this matters. CBOR support, on the other hand does matter. I vote to CONDITIONALLY ACCEPT Boost.JSON, conditional on the inclusion of code for parsing and serializing boost::json::value as CBOR. I can live with the added verbosity of Boost.JSON (although I'd rather see it reduced where possible), but not without CBOR. (If CBOR support is not added, this vote should count as an abstain vote and not as a reject. I don't think that Boost.JSON needs CBOR in order to be useful to other people - I just don't want to vote to accept a library that's not useful for me.) -- Rainer Deyke (rainerd@eldwood.com)