Re: Boost.Charconv review runs January 15th - 25th, 2024

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2024-01-23 19:03:32

pon., 22 sty 2024 o 21:58 Ruben Perez via Boost <boost_at_[hidden]>
napisał(a):

> 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/detail/fast_float/fast_float.hpp#L14

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/workflows/build-code.yml
> 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
>