Re: Boost.Charconv review runs January 15th - 25th, 2024

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.