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

wt., 23 sty 2024 o 14:38 Christopher Kormanyos via Boost < boost@lists.boost.org> 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 wrote:

On Monday, January 22nd, 2024 at 9:57 PM, Ruben Perez via Boost < boost@lists.boost.org> 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;