## 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/41a83261c0f65aee575e89ccb71d94b... and https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b... and https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b... 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/41a83261c0f65aee575e89ccb71d94b... 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/41a83261c0f65aee575e89ccb71d94b... * https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b... * https://github.com/cppalliance/charconv/blob/41a83261c0f65aee575e89ccb71d94b... ## 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