From: John Maddock (jz.maddock_at_[hidden])
Date: 2024-01-25 17:24:38
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?
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/b99d82948918df08d0acdc1c3ef5040ab3d19566/src/from_chars.cpp#L35
along with some one line forwarding functions:
https://github.com/cppalliance/charconv/blob/b99d82948918df08d0acdc1c3ef5040ab3d19566/src/from_chars.cpp#L308
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.
> - 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
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.