Re: Review of proposed CharConv

From: Matt Borland (matt_at_[hidden])
Date: 2024-01-25 07:38:52

> Fine in general. A few nitpicks/concerns follow.
> 

> 

> ### Arrays of constants
> 

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

I can look into it. I know there is some overlap with the standard libraries but not completely. For example I know libstdc++ uses fast_float directly, but MSVC and libc++ do not.

> 

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

Andrzej brought this up as well. See: https://github.com/cppalliance/charconv/issues/126.

> 

> ### 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/
> 

I can look into this as well. This is not the first time this has been brought up.

Thank you for the review and feedback. I'll address the rest of the nitpicks.

Matt