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/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'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