FP formatting at compile-time #2426

alexezeder · 2021-07-16T20:25:19Z

Few points for this PR, I mentioned some of them in #1895 (comment):

works only with FMT_HEADER_ONLY
works only with fast_floats, even when specs passed, because of the sign check
separate compile-fp-test added with HEADER_ONLY argument (tested via G++11 C++20 and MSVC 2019 C++20 configs on CI), the compile-test still uses static(/shared?) library

DanielaE

There seem to be a few general portability issues that need to be addressed.

include/fmt/core.h

include/fmt/format-inl.h

include/fmt/format.h

test/CMakeLists.txt

include/fmt/format-inl.h

vitaut

Thanks for the PR!

.github/workflows/linux.yml

include/fmt/core.h

include/fmt/format-inl.h

alexezeder · 2021-07-25T20:17:30Z

JFI, I will get back on this PR about a week later.

alexezeder · 2021-08-12T16:43:37Z

JFI, I'm back on this PR.
God, FO4 has too many sidequests...

vitaut

Could you move bigint constexprification to a separate PR since it's pretty straightforward?

include/fmt/format.h

vitaut · 2021-08-26T19:29:12Z

Thanks for the update, I'll take a look shortly. As a general comment, could you move all straightforward additions of constexpr (that don't remove static) such as the ones in fmt/core.h, fp, bigint, etc. to a separate diff? Those can be merged without much discussion and it will simplify reviewing the current diff. CI changes can go there too.

alexezeder · 2021-08-26T22:14:36Z

As a general comment, could you move all straightforward additions of constexpr (that don't remove static) such as the ones in fmt/core.h, fp, bigint, etc. to a separate diff?

Done with #2470. I will rebase this PR then, as soon as the new one is merged.

alexezeder · 2021-08-28T23:31:39Z

Hmm... looks like we have problems with digit_grouping in the write_float() since it's used without is_constant_evaluated() check and probably because it contains std::string, which is not constexpr-friendly (but it should be, right? 🤔). I tried to solve this by using the code from few commits back under is_constant_evaluated() check, since it doesn't use digit_grouping, but this solution only works for MSVC, while GCC still has some problems with this function.

include/fmt/format-inl.h

include/fmt/format.h

test/CMakeLists.txt

include/fmt/format.h

vitaut · 2021-08-29T17:55:18Z

Hmm... looks like we have problems with digit_grouping in the write_float() since it's used without is_constant_evaluated() check and probably because it contains std::string, which is not constexpr-friendly (but it should be, right? 🤔)

We could probably create a fallback digit_grouping for the consteval case.

alexezeder · 2021-08-29T21:35:41Z

We could probably create a fallback digit_grouping for the consteval case.

Without creating a fallback, but I managed to make it compile on GCC 11.

vitaut

Mostly looks good, a few more comments inline.

include/fmt/format-inl.h

include/fmt/format.h

vitaut · 2021-09-02T13:52:18Z

include/fmt/format.h

+    } else {
+      FMT_ASSERT(false, "floating point type is not supported");
+    }


Can if + assert be replaced with a static_assert?

It looks like no because the code under is_constant_evaluated() check is still a part of function code. Without optimization, it's not even omitted, according to Compiler Explorer. So this constant-evaluated code has to be valid, and when it contains static assert, which is false, then the "runtime" version of this function becomes ill-formed.

Sure but at the very least we could drop the else branch because non-IEEE is very uncommon and we would get a reasonable error anyway from signbit below.

I see you dropped it from isinf and isfinite but not here.

I see you dropped it from isinf and isfinite but not here.

Only because you wrote:

we would get a reasonable error anyway from signbit below.

and it's signbit.

If this line is about the std::signbit() call below, then we won't get a reasonable error from there in some cases when FP numbers are non-IEEE and std::signbit() manages to work with them.

we won't get a reasonable error from there in some cases when FP numbers are non-IEEE and std::signbit() manages to work with them

That seems fine to me. We'll either get an error if signbit is not constexpr or it will return the correct result.

Removed also here.

include/fmt/format.h

vitaut · 2021-09-10T13:12:21Z

include/fmt/format.h

  if (const_check(!is_supported_floating_point(value))) return out;
  float_specs fspecs = parse_float_type_spec(specs);
  fspecs.sign = specs.sign;
-  if (std::signbit(value)) {  // value < 0 is false for NaN so use signbit.


Could you keep the comment since it may not be obvious to the reader why we don't use < 0 here?

This comment reverted back.

vitaut · 2021-09-10T13:12:56Z

include/fmt/format.h

@@ -1886,22 +1897,95 @@ auto write_float(OutputIt out, const DecimalFP& fp,
  });
 }

+template <typename Char> class fallback_digit_grouping {
+ public:
+  explicit constexpr fallback_digit_grouping(locale_ref, bool) {}


I don't think it needs to be explicit.

Yeah, it's just copied from digit_grouping, done.

vitaut

A few more nits, otherwise LGTM.

alexezeder · 2021-09-15T08:40:26Z

I will address these nits and resolve merge conflicts tomorrow.

* works only with FMT_HEADER_ONLY * works only with float and double types (not long double)

alexezeder · 2021-09-17T14:55:26Z

Unfortunately, my continuous benchmark pages broke at some point (June 10). I will try to fix them soon since I have access to this Raspberry finally 😄.

vitaut · 2021-09-18T15:03:33Z

Merged, thanks!

* works only with FMT_HEADER_ONLY * works only with float and double types (not long double)

DanielaE reviewed Jul 18, 2021

View reviewed changes

include/fmt/core.h Outdated Show resolved Hide resolved

include/fmt/format-inl.h Outdated Show resolved Hide resolved

include/fmt/format.h Show resolved Hide resolved

test/CMakeLists.txt Show resolved Hide resolved

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch from ec4e993 to 5bdc572 Compare July 19, 2021 00:31

DanielaE reviewed Jul 20, 2021

View reviewed changes

include/fmt/format-inl.h Show resolved Hide resolved

alexezeder marked this pull request as draft July 20, 2021 15:37

vitaut requested changes Jul 23, 2021

View reviewed changes

.github/workflows/linux.yml Outdated Show resolved Hide resolved

include/fmt/core.h Outdated Show resolved Hide resolved

include/fmt/format-inl.h Outdated Show resolved Hide resolved

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch from 5bdc572 to 30f17cc Compare August 12, 2021 16:22

vitaut requested changes Aug 15, 2021

View reviewed changes

include/fmt/format.h Outdated Show resolved Hide resolved

include/fmt/format.h Outdated Show resolved Hide resolved

include/fmt/format.h Outdated Show resolved Hide resolved

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch 2 times, most recently from 7087b17 to 2f74e6c Compare August 23, 2021 19:26

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch 2 times, most recently from f645dc0 to ff0cf70 Compare August 28, 2021 22:50

vitaut requested changes Aug 29, 2021

View reviewed changes

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch 2 times, most recently from ae1aeca to 5911f4f Compare August 29, 2021 21:11

alexezeder marked this pull request as ready for review August 29, 2021 21:35

alexezeder requested a review from vitaut August 29, 2021 21:36

vitaut requested changes Sep 2, 2021

View reviewed changes

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch from 5911f4f to 9285127 Compare September 2, 2021 19:20

alexezeder requested a review from vitaut September 2, 2021 19:29

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch 3 times, most recently from ecdd2a8 to 0f18453 Compare September 7, 2021 20:12

vitaut reviewed Sep 10, 2021

View reviewed changes

make FP formatting available to be used at compile-time

ea86c06

* works only with FMT_HEADER_ONLY * works only with float and double types (not long double)

alexezeder force-pushed the feature/FP_formatting_at_compile_time branch from 0f18453 to ea86c06 Compare September 17, 2021 14:42

vitaut approved these changes Sep 18, 2021

View reviewed changes

vitaut merged commit b4d9d82 into fmtlib:master Sep 18, 2021

alexezeder mentioned this pull request Sep 22, 2021

Is there a specific reason to not constexprify Dragonbox? #2506

Closed

jk-jeon mentioned this pull request Sep 22, 2021

Add static to a table #2509

Merged

PoetaKodu pushed a commit to pacc-repo/fmt that referenced this pull request Nov 11, 2021

make FP formatting available to be used at compile-time (fmtlib#2426)

aff006d

* works only with FMT_HEADER_ONLY * works only with float and double types (not long double)

majetideepak mentioned this pull request Dec 7, 2023

Make Velox compatible with fmt >= 9 facebookincubator/velox#7896

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FP formatting at compile-time #2426

FP formatting at compile-time #2426

alexezeder commented Jul 16, 2021 •

edited

Loading

DanielaE left a comment

vitaut left a comment

alexezeder commented Jul 25, 2021

alexezeder commented Aug 12, 2021

vitaut left a comment

vitaut commented Aug 26, 2021

alexezeder commented Aug 26, 2021 •

edited

Loading

alexezeder commented Aug 28, 2021

vitaut commented Aug 29, 2021

alexezeder commented Aug 29, 2021

vitaut left a comment

vitaut Sep 2, 2021

alexezeder Sep 2, 2021

vitaut Sep 3, 2021

alexezeder Sep 3, 2021

vitaut Sep 3, 2021

alexezeder Sep 6, 2021

alexezeder Sep 7, 2021

vitaut Sep 10, 2021

alexezeder Sep 17, 2021

vitaut Sep 10, 2021

alexezeder Sep 17, 2021

vitaut Sep 10, 2021

alexezeder Sep 17, 2021

vitaut left a comment

alexezeder commented Sep 15, 2021

alexezeder commented Sep 17, 2021

vitaut commented Sep 18, 2021

FP formatting at compile-time #2426

FP formatting at compile-time #2426

Conversation

alexezeder commented Jul 16, 2021 • edited Loading

DanielaE left a comment

Choose a reason for hiding this comment

vitaut left a comment

Choose a reason for hiding this comment

alexezeder commented Jul 25, 2021

alexezeder commented Aug 12, 2021

vitaut left a comment

Choose a reason for hiding this comment

vitaut commented Aug 26, 2021

alexezeder commented Aug 26, 2021 • edited Loading

alexezeder commented Aug 28, 2021

vitaut commented Aug 29, 2021

alexezeder commented Aug 29, 2021

vitaut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut left a comment

Choose a reason for hiding this comment

alexezeder commented Sep 15, 2021

alexezeder commented Sep 17, 2021

vitaut commented Sep 18, 2021

alexezeder commented Jul 16, 2021 •

edited

Loading

alexezeder commented Aug 26, 2021 •

edited

Loading