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

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2024-01-23 16:50:08

wt., 23 sty 2024 o 14:38 Christopher Kormanyos via Boost <
boost_at_[hidden]> napisał(a):

>  > The review of Boost.Charconv by Matt Borland runs> Monday, January 15th
> through January 25th, 2024.
>
> Dear all,
> This post is intended to kindly encourage thesubmission of more reviews
> for the proposedBoost.Charconv.
> We've had thoughtful, deep discussion,
> but we really need a few more reviews to form
> a solid basis for results. The review periodis formally scheduled until
> 25-January-2024.
>
> Your review can be brief, and handle someof the major queries. A terse
> review is probablybetter than an absent one.
> Thank you for considering the proposedBoost.Charconv.
>
> Christopher
>     On Tuesday, January 23, 2024 at 08:43:36 AM GMT+1, Matt Borland via
> Boost <boost_at_[hidden]> wrote:
>
>
>
>
>
>
>
> On Monday, January 22nd, 2024 at 9:57 PM, Ruben Perez via Boost <
> boost_at_[hidden]> wrote:
>
> >
>
> >
>
> > Hi all,
> >
>
> > This is my review of the proposed Boost.Charconv.
> >
>
>
> Reuben,
>
> Thanks for taking time to review.
>
> >
>
> > I’ve only had a quick glance. It surprised me finding an instance of
> > malloc/strtold, since these functions violate the “non-allocating,
> > locale-independent” guarantees. I understand this is for an extreme
> > edge case, and that the “locale-independent” guarantee is still
> > provided through internal logic. I don’t know enough about these
> > parsing algorithms to determine whether this is the right decision or
> > not. How much effort would require rolling a hand-rolled strtold
> > alternative for such edge cases? In any case, this can be done once
> > the library is in Boost.
> >
>
>
> The locale issue from last week is fixed on develop and master. strtold
> and strtof128 from scratch will be non-trivial additions to the library.
> The malloc could easily be dropped by rejecting strings in fallback over
> 1024 bytes, but that seems like the worse option of the two for now.
>
>
> > 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.
> >
>
>
> I think Klemens has multi-page asciidoc libraries. I'll look at how he
> does that.
>
>
> > 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.
> >
>
>
> I will add the fuzzing, and I am sure I will ask questions as I have never
> used it before.
>
>
> > 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.
>

Hi Everyone,
This is not really a review, but I want to report an issue I have with this
library on an ideological level.

I mean the behavior with assigning zero and `HUGE_VAL` upon
result_out_of_range.
https://github.com/cppalliance/charconv/issues/110

When you do it, you cannot claim that this library is "an Implementation of
<charconv> in C++11".
Peter informs us about this issue:
https://cplusplus.github.io/LWG/lwg-active.html#3081
But having an issue in the Standard doesn't automatically mean that the
standard will change in the way recommended in that report.

This has practical implications on the users: I cannot just substitute
std::from_chars for boost::from_chars (in order to increase performance),
because the semantics of my program will slightly change.
boost::from_chars_strict
may be a drop-in replacement for std::from_chars. boost::from_chars is not.

We recently had a discussion that boost::optional is not a substitute for
std::optional, because they have different interfaces; neither is
boost::variant2 for std::variant: you cannot interchange them without
consequences. It looks like boost::from_chars is heading for the same
category.

Can I request that this library flips the defaults, so that
boost::from_chars is 100%-compatible with std::from_chars, and you also get
boost::from_chars_plus that behaves like strtod?

I hope that this message does not come across as negative. Matt, I can only
imagine how much work went into writing this library. Thank you for doing
this and sharing it.
The speedups indicated in the benchmark section make the library worth
using. I never believed anything can outperform Boost.Spirit.

Regards,
&rzej;