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.
- `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
- "`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
> - What is your evaluation of the potential usefulness of the library?
Very useful especially after reading Peters review about extending the
> - 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.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk