[review][JSON] Review of Alex

From: Alexander Grund (alexander.grund_at_[hidden])
Date: 2020-09-21 09:03:59

> 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).
ACCEPT. Few minor issues to iron out, but nothing holding back acceptance.
> Some other questions you might want to consider answering:
>
>    - What is your evaluation of the design?
Sound. The introduction of a new string-type raises eyebrows but it 
seems to be worth it given the improvements. However a conversion to a 
std::string should likely be added (or documented as supported already)
I'm however not convinced about the as_double, value_to<double> and 
number_cast methods and their differences and intuitive exceptions from 
users. I'd expect that documented at "Using numbers" or at least a cross 
reference from there
>    - What is your evaluation of the implementation?
After the discussions in Slack and on ML it improved considerably. 
Especially due to use of inline namespaces and is_/as_ function changes. 
Things left:
- `basic_parser` being declared in a detail header feels wrong. The 
implementation (which includes the "private" declaration) is "public" 
which is kind of the opposite I expected and indeed other reviewers 
found the same. Especially as the docs list it as "defined in detail". 
I'd put both in public, maybe use "basic_parser_impl.hpp"
- There seem to be multiple ways of doing things and some seem to make 
great differences in exception handling, conversions and performance. 
Sometimes this doesn't seem to be clear (see other reviews and above)
>    - What is your evaluation of the documentation?
Very good with few improvements:
- "Quick Look" should be a top-level link. I struggled to find it in my 
first pass. I'd expect that to be the very first link when opening the 
docu or even at the front-page
- "This storage is freed when the parser is destroyed, allowing the 
parser to cheaply re-use this memory on subsequent parses, improving 
performance." - What does this mean? How can a destroyed parser reuse 
anything? Or why does it need highlighting that freed memory can be reused?
- The example `handler` assigns "-1" to a std::size_t. I guess that 
deserves a comment at least for beginners wondering about the 
signed->unsigned conversion
- "`finish` Parse JSON incrementally." is lacking a clearer summary
- "parser::release" says "UB if the parser is not done" but "If ! 
this->done(), an exception is thrown.", which seems contradictory. I'd 
remove the UB here, maybe use an EC overload if exceptions should be avoided
- "write/write_some" both say "Parse JSON incrementally.". The summary 
should make it clearer what they do and the full description should 
contain, well, a full description
- concepts like "ToMapLike" are explained after they are used w/o a 
link/reference
>    - What is your evaluation of the potential usefulness of the library?
Very useful especially after reading Peters review about extending the 
parser etc.
>    - Did you try to use the library? With which compiler(s)? Did you have
> any problems?
Yes with some small tests. GCC 10.0, no problems
>    - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?without explanation what they are
Couple hours checking docu and code.
>    - Are you knowledgeable about the problem domain?
Ok, I guess. Just as anyone who has used JSON anywhere.