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

From: Matt Borland (matt_at_[hidden])
Date: 2024-01-23 07:43:02

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.
> 

See: https://github.com/cppalliance/charconv/issues/131

Matt