With apologies to Matt, this is a somewhat limited review based on limited time, but I hope it's useful non-the-less. I should also declare that I know Matt through his excellent help with the Math library.
Please optionally provide feedback on the followinggeneral topics:
- What is your evaluation of the design?
It is what it is, nothing to say here.
- What is your evaluation of the implementation?
- What is your evaluation of the documentation? I'm reasonably happy I can find what I need with the docs, but would echo the more detailed comments of others. It would be nice if the code snippet at the very start was actually something I could cut and paste and run "as is" though. - What is your evaluation of the potential usefulness of the library? Actually, I'm not sure - it is super useful, but it would interesting to see how quickly native C++20 support gains traction - I can see other
Matt has clearly worked hard to ensure this uses the current state of the art implementations - the performance figures speak for themselves here. I'm also impressed with the amount of testing going on - I know this is a "small" library - at least in it's interface - but testing this stuff is hard, and pretty vital too. On the specifics: I agree with the comment about the constants in the headers, while these could hidden inside inline functions (which would then presumably get merged at link time), I see no good reason not to declare them extern and move to a .cpp file, given that we have separate source anyway. I agree with the comment about moving implementation only headers to the src directory. With regard to behaviour, I can see that we can't please everyone here. At present, I would weakly vote for bleeding-edge behaviour to be the default (all pending defects in the std wording fixed), but I can imagine situations where different projects would have differing requirements. Thinking out loud here, can we use namespaces to help here: namespace boost{ namespace charconv{ namespace cxx20{ // C++ 20 interface here } namespace latest{ // bleeding edge here } using from_chars = whatever; }} The "whatever" here, could potentially be macro configured as Matt had it before. I also prefer a name like "cxx20" to "strict" as strict compared to which standard? This would also allow users to fix behaviour by explicitly calling say cxx20::from_chars which would give them consistent behaviour going forward (whatever future standards do), and is nicely self documenting too. Am I missing something here, seems to easy and I'm surprised no one has suggested this? In from_chars.cpp source, I'm seeing quite a few fairly short functions https://github.com/cppalliance/charconv/blob/b99d82948918df08d0acdc1c3ef5040... along with some one line forwarding functions: https://github.com/cppalliance/charconv/blob/b99d82948918df08d0acdc1c3ef5040... Is there any particular reason why these are not declared inline in the headers? I created a short test program to test every possible value of type float (this works for float, don't ever try it double!): #include <boost/math/special_functions/next.hpp> #include <boost/charconv.hpp> int main() { float f = std::numeric_limits<float>::min(); while (f != std::numeric_limits<float>::max()) { char buffer[128]; boost::charconv::to_chars_result r = boost::charconv::to_chars(buffer, buffer + 128, f); assert(r.ec == std::errc()); *r.ptr = 0; float ff; boost::charconv::from_chars_result rr = boost::charconv::from_chars(buffer, buffer + std::strlen(buffer), ff); assert(rr.ec == std::errc()); assert(f == ff); f = boost::math::float_next(f); } return 0; } I initially did not have the *r.ptr = 0; line in there which was of course a bug on my part - one that's ever so easy to fall into, I'll like to see a big bold declaration in the docs noting that to_chars does not null-terminate the string. I have a hunch this will catch a few people out! In any case this completes successfully, which looks good to me. One minor annoyance, is that if I build the library from my IDE without defining BOOST_CHARCONV_SOURCE manually then it tries to auto-link to itself! It would be a nice convenience to put that define at the top of each source file. libraries using it though to avoid ties to C++20.
- Did you try to use the library? With what compiler? Did you have any problems? Only with latest MSVC. I note that Matt has a comprehensive set of CI tests though, so I'm happy portability is there. It would be nice to see some non-Intel architectures in there just to check the bit-fiddling code, but other than that it's looking good. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? About 6 or 7 hours, mostly looking at code and running some quick tests. - Are you knowledgeable about the problem domain?
Somewhat, Paul Bristow and I discovered some bugs in std lib floating point to string conversions years ago, so I do know how to torture this kind of code. The actual routines used are all new to me though. And finally, Yes I do think think this should be accepted into Boost, subject to comments (as usual). Best, John.