Constexpr to_decimal - Githubissues

jk-jeon / dragonbox

Reference implementation of Dragonbox in C++

Apache License 2.0

607 stars 39 forks source link

Constexpr to_decimal #42

Closed leni536 closed 1 year ago

leni536 commented 1 year ago

Attempt to make to_decimal constexpr when compiled in C++20. Added some smoke tests, a more comprehensive set of constexpr tests would be needed.

jk-jeon commented 1 year ago

Looks very good overall👍 The issue you pointed out might require some restructuring of the code but I will do that later.

leni536 commented 1 year ago

I disabled the constexpr test by default, I think the current compilers used for testing are not implementing all the features I'm relying on.

I manually tested on gcc 12 and clang 15, it passes there. Could use a workflow that used an up-to-date compiler and constexpr test enabled.

jk-jeon commented 1 year ago

I disabled the constexpr test by default, I think the current compilers used for testing are not implementing all the features I'm relying on.

I manually tested on gcc 12 and clang 15, it passes there. Could use a workflow that used an up-to-date compiler and constexpr test enabled.

Is it that they do have C++20 modes while they don't really implement the required features? (Because otherwise it should fail even before the compiling begins, as the CMake script requries C++20.)

Anyway I'll look into ways to update the CI. You can disable the test for the moment. Thanks!

leni536 commented 1 year ago

I disabled the constexpr test by default, I think the current compilers used for testing are not implementing all the features I'm relying on. I manually tested on gcc 12 and clang 15, it passes there. Could use a workflow that used an up-to-date compiler and constexpr test enabled.

Is it that they do have C++20 modes while they don't really implement the required features? (Because otherwise it should fail even before the compiling begins, as the CMake script requries C++20.)

Anyway I'll look into ways to update the CI. You can disable the test for the moment. Thanks!

According to cppreference, gcc supports bit_cast from version 11. I believe cmake happily sets -std=c++2a when you ask for C++20, where the option means "experimental C++20 support for the upcoming standard" from gcc 10's point of view.

leni536 commented 1 year ago

I consider this PR ready. Roadmap for PRs that will follow:

Make to_chars constexpr. I have a trick up my sleeve for to_chars and handling the separate translation unit, hopefully it's valid C++.
Make the Ryu implementation constexpr for testing.
Add constexpr tests for existing test cases.

jk-jeon commented 1 year ago

Merged, thanks!