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

From: Matt Borland (matt_at_[hidden])
Date: 2024-01-25 17:51:36

> 

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

I will address these both in post-review re-factoring.

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

The only issue I have with this is "latest" isn't tied to a specific C++ standard but what we think is the best resolution to an issue with the way the standard is written at current. At some point the latest draft of the standard will have a resolution (in theory at least).

> 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'll hit these too in refactoring.

> 

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

ARM64 and s390x are in the drone run (https://github.com/cppalliance/charconv/blob/develop/.drone.jsonnet#L135). These both have native 128-bit long doubles, and s390x covers big-endian arch.

> 

> Yes I do think think this should be accepted into Boost, subject to
> comments (as usual).
> 

Thanks as always John. I appreciate the time and feedback.

Matt