pon., 22 sty 2024 o 21:58 Ruben Perez via Boost
Hi all,
This is my review of the proposed Boost.Charconv.
- What is your evaluation of the documentation?
I find it really difficult to navigate. Fortunately, it closely mimics the standard, so I end up just looking at cppreference. But still, my experience hasn’t been good. These are the main pain points I see:
- It's a single page. Even for a small library, it makes navigation very cumbersome. - It’s not linked. If you want to find the reference for a struct or function, your only option is to Ctrl+F. - The overview for each function is written like a reference. But then you have another reference at the bottom. - The sidebar TOC has too many levels.
Interestingly, I found this comment in the implementation: https://github.com/cppalliance/charconv/blob/develop/include/boost/charconv/... And I feel it describes way nicer what the library (at least this function) does. Regards, &rzej;
As much as I want to get rid of quickbook, my feeling is that quickbook-based docs currently generate more usable output than asciidoc-based ones. It may be a matter of tuning templates and developing additional tools, but we’re not there yet.
Please find some additional minor comments at the end of the review.
- Did you try to use the library? With what compiler? Did you have any problems?
I’ve integrated the library into Boost.MySQL, in a feature branch (https://github.com/boostorg/mysql/tree/feature/boost-charconv). I’ve fully implemented query formatting and updated the parsing functions, all using the proposed library. I’ve had no surprises. All CIs pass all tests, which perform considerable coverage on the parsing side. This includes different versions of the major compilers and operating systems, with different configurations - see https://github.com/boostorg/mysql/blob/feature/boost-charconv/.drone.star and https://github.com/boostorg/mysql/blob/feature/boost-charconv/.github/workfl... for details.
I’ve found a problem when linking to the library using the CMake targets, where a missing flag causes linker errors - see https://github.com/cppalliance/charconv/issues/130.
- How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
I’ve dedicated around 15 hours of work to integrate the library in Boost.MySQL and clean up CIs.
- Are you knowledgeable about the problem domain?
I’m just a user. I understand the domain problem and the different alternatives. I don’t have any knowledge about the algorithms’ internal workings.
- Your decision.
I recommend CONDITIONALLY ACCEPT Charconv into Boost, under the condition that proper fuzzing is added to the test suite. I’ve worked with libfuzzer before and can assist the author if required.
Thanks Matt for submitting the library, and Christopher for being review manager.
Regards, Ruben.
Minor comments on documentation:
- The first example uses to_chars(... sizeof(buffer) - 1), which seems to imply a NULL-terminator somewhere. Since there is none, I think it should just be sizeof(buffer). - Docs don't specify the behavior about NULL-terminators. Either specify "adheres to standard" or clarify that no NULL-terminator is written. - IMO the front page should clearly state that this library is compliant with the charconv standard header, noting any exceptions. - from_chars overview is missing semicolons in the friends section. - Please use monospace in the discussion to refer to identifiers, like [first, last). - Basic usage: from_chars name is qualified but from_chars_result is not. - Code snippets wrap, which makes them difficult to read.
_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost