21 Sep
2020
21 Sep
'20
9:03 a.m.
> 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_toand 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.