constexpr
Closed this issue · 8 comments
I recently contributed to https://github.com/fastfloat/fast_float to make the from_chars implementation there constexpr from C++20.
I think making to_chars constexpr is equally possible. Would efforts to do this be welcome here?
I see that to_chars is currently defined in a .cpp file, which could make this challenging without making the library header-only.
Hi, that would be great.
I was also planning to do that at some point, but there are several issues.
First of all, as you pointed out, the library is currently not header-only. My original intention was to move as many things as possible into a separate TU to reduce compile time, but at this point I think maybe the portion dragonbox_to_chars.cpp takes might be tiny compared to dragonbox.h in terms of compile time. But I still wanted to be sure about that so planned to do some benchmark but never did it yet.
The second issue is the language standard. I think it is probably close-to-impossible to make everything constexpr without performance regression within C++17, so my plan was to support constexpr only in C++20 and further. I anyway wanted to support C++11/14, so the plan was to introduce some macros depending on the language version, e.g., something like
#if __cplusplus >= 202300L // Don't know the precise value
#define JKJ_DRAGONBOX_IS_CONSTANT_EVALUATED if consteval
#elif __cplusplus >= 202002L
#define JKJ_DRAGONBOX_IS_CONSTANT_EVALUATED if (std::is_constant_evaluated())
#endif
But robust handling of these stuffs might be tricky for number of reasons, for instance:
- Some compiler might report
__cplusplusto be high enough but still lacks some features. And the proper support of feature testing macros had been quite messy among older versions of the compilers. - MSVC reported a wrong value of
__cplusplusfor quite a while, maybe we need to introduce a yet another level of indirection here. - I am not quite sure how to edit my CI to properly test various language versions.
Oh, I just realized you made a PR about this. I will make some comments on that. Thanks a lot!
2. MSVC reported a wrong value of
__cplusplusfor quite a while, maybe we need to introduce a yet another level of indirection here.
MSVC still reports the wrong value, you have to use a switch for the correct value. The workaround is this:
#if __cplusplus >= 201703L || (defined(_MSVC_LANG) && _MSVC_LANG >= 201703L)@alugowski Ah, thanks for the info!
I recently contributed to https://github.com/fastfloat/fast_float to make the
from_charsimplementation there constexpr from C++20.I think making
to_charsconstexpr is equally possible. Would efforts to do this be welcome here?I see that
to_charsis currently defined in a .cpp file, which could make this challenging without making the library header-only.
It's possible, I have an implementation of it from an old fork in JSON Link that is constexpr
@beached Hi Darrell,
After #42, now dragonbox::to_decimal is already constexpr in C++20+. The remaining issue is that dragonbox::to_chars isn't constexpr, which is essentially because the implementation lives in a separate .cpp file. (Technically testing and other stuffs are also not constexpr but I don't find any reason to make them constexpr anyway.)
I don't really want to make to_chars feature header-only, and also don't want to have a configuration macro for header-only/compiled dual support. (Welcome to ODR hell!)
What I'm thinking is that maybe we can test if consteval (or std::is_constant_evaluated) inside dragonbox::to_chars and if that's the case, then call another implementation living in a header file which just does the naive no-brain digit printing without doing any jeaiii nonsense. So in a sense, it's header-only in the constexpr context, and is compiled otherwise.
What hinders me at this point is that,
- @leni536 said he has a trick for
constexpr-ifyingdragonbox::to_charsbut didn't come back so far, - I don't want to copy-paste all the language version testing macros and such stuffs from
dragonbox.hintodragonbox_to_chars.h. Then I have to either leak some of the macros into the user's code, or have to have some more support headers, so that now the premise of being single header is broken. For the first point, I personally hate libraries leaking many implementation detail macros and I believe that ideally the only macros exposed to the users are the header guards and interface macros that are explicitly intentionally exposed to the users. For the second point, I guess I will eventually break downdragonbox.hinto several headers anyway as the project has grown beyond my initial expectation, but I am still indecisive of what would be the best splitting.
Nice! I'll add that to my list to look at as I don't use to_chars but just to_decimal and do a custom formatting for JSON.