Review of proposed CharConv

From: Antony Polukhin (antoshkka_at_[hidden])
Date: 2024-01-24 19:48:28

## What is your evaluation of the design?

Design follows the C++ Standard library design with all the latest
additions. Everything is fine.

## What is your evaluation of the implementation?

Fine in general. A few nitpicks/concerns follow.

### Arrays of constants

RYU/fast_float/... are used for some of the conversions and most of those
algorithms have huge tables full of constants. For example
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/ryu/generic_128.hpp#L31
and
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/from_chars_integer_impl.hpp#L25
and
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/to_chars_integer_impl.hpp#L31C23-L31C34

Those constants have internal linkage. They are put in every
translation unit that includes and uses them. Those constants would not be
merged by
linker. Moreover, the same constants with different names now exist in
Standard Library implementations and are not merged with Boost.Charconv.

To reduce the binary size and improve data locality I'd recommend switching
to
std implementation in sources if the feature test macro tells that
std::to_chars/std::from_chars are fine.

### Internal headers in include/

Some of the headers are used only internally during build. It seems
reasonable
to move them to the src/ directory to make sure that no one uses them and
that the library itself does not include them by accident.

### Header only support

There is a PR to make the library header only
https://github.com/cppalliance/charconv/pull/138

This feature is demanded by users however it raises the constants question
again.
Marking arrays of constants with `inline` should be done to make the
constants
externally visible, so that the linker could merge them.

### Constexpr for floats

Just a feature request - it would be nice to have constexpr from_chars
and constexpr to_chars for floating point types. If is_constant_evaluated()
- do
slow but constexpr conversion. Otherwise - fallback to std:: version or
to the implementation in src/

### Locales

Locales are used in some corner cases along with switching them back and
forth
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/from_chars_float_impl.hpp#L64

Looks like there's no need in that line for locales switching. Just get the
decimal point separator from the current locale and if it is not '.' then
do the replacement of the first '.' to the locale's separator.

### Nitpicks

Looks like zero initialization of the buffers could be skipped in following
places:
*
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/to_chars_integer_impl.hpp#L89
*
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/to_chars_integer_impl.hpp#L275C10-L275C16
*
https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b9ddcf799b/include/boost/charconv/detail/to_chars_integer_impl.hpp#L351

## What is your evaluation of the documentation?

Looks like parts of the docs repeat themself. For example, the reference
section
duplicates some of the previous descriptions. The latter could be just
removed.

Another thing that catches an eye are the benchmarks. Please also provide
some
absolute numbers because "X is five times faster than Y" could easily be
that
X and Y execute for ~1ns each. Those numbers are profitable for users.

Side Note for myself: Seems that LexicalCast shows poor performance in some
cases where it should not. Will investigate.

## What is your evaluation of the potential usefulness of the library?

Very useful for libraries that wish to support pre-C++17 standards or a
wide range of compilers. Useful for experimenting and prototyping new C++
Standard Library proposals

## Did you try to use the library? With what compiler? Did you have any
problems?
## How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

Just looked through the code and the docs.

## Are you knowledgeable about the problem domain?

I'm maintaining the Boost.LexicalCast and implemented a bunch of
optimizations for std::to_chars/std::from_chars for the libstdc++. I'm not
knowledgeable of RUY internals, but have the general understanding of
the <charconv>.

## Do you think the library should be accepted as a Boost library?

Yes