Add support of most format_specs for formatting at compile-time #2056

alexezeder · 2020-12-09T01:28:12Z

Extends #2019 by adding support of formatting specs for bool, integer, char and string:

fill <a character other than '{' or '}'>, like {:*>4}, {:ж^4}
align "<" | ">" | "^", like {:^4}
sign "+" | "-" | " ", like {:+}
"#", like {:#b}
"0", like {:04}
width integer | "{" "}", like {:4}, {:{}}
int_type "b" | "B" | "d" | "o" | "x" | "X", like {:b}, {:x}, {:X}.

I think that bool, integer, char and string formatting should be fully supported at compile-time, of course, if there are no caveats I'm not aware of. Floating-point formatting is not addressed by this PR, so it's still unavailable at compile-time.

Also test_string in compile-test updated a bit to use not std:: but fmt:string_view, this eliminates unnecessary include there

…st_string

alexezeder · 2020-12-10T11:09:14Z

Also, looks like tests for formatting at compile-time (CompileTimeFormattingTest in compile-test.cc) do not run on CI because the GCC 10.2 defines __cplusplus less than 202002L, at least on my PC... and on Compiler Explorer too, so CI definitely doesn't run it.
I suggest adding define for this whole formatting at compile-time feature in the same manner as with other features, so if it's not predefined then set it by the expression we already have __cplusplus >= 202002L. Is it a good idea, should I do that in this PR or not?

vitaut

Thanks for the PR!

vitaut · 2020-12-12T15:55:24Z

include/fmt/compile.h

-  OutputIt format(OutputIt out, const Args&... args) const {
+  constexpr OutputIt format(OutputIt out, const Args&... args) {


What exactly is the problem with const?

All these problems started with mutable, GCC thinks that mutable formatter<T, Char> fmt in spec_field structure cannot be used at compile-time. As I can see Clang <7.0 also thinks the same way, this is why the side effect of provided PR is that Clang <7.0 with C++17 support can also use FMT_COMPILE with specs.

I failed to get a small code example which raises this error, I tried to create a failing example by recreating all significant parts of the error stack trace, but strangely it works. But I can present you the huge example with this error, this example is created by combining all needed headers of {fmt} together (version from this PR, except compile.h) and by making everything needed as constexpr in compile.h without mutable-related changes. Here it is on Compiler Explorer: https://godbolt.org/z/n1Kz38

Since the behaviour of GCC 10.2 and Clang <7.0 is kind of similar, I also tried to recreate the error with Clang 6.0 with my small code example that IMHO should fail - no luck, so looks like I'm missing something here. But of course, it fails to compile with a bit modified huge example where all {fmt} headers are combined: https://godbolt.org/z/P17TGG

So, this became non-const to eliminate mutable, const CompiledFormat& cf became non-const references because method format is no more const, constexpr auto compiled became just auto compiled because const object cannot be passed via non-const reference. This is my explanation for why I made these changes, of course, I can miss some more elegant solution, especially considering that I failed to recreate this error in a small example. In case it's true and more elegant solution exist I'll be glad to implement it.

I think the correct solution is to make formatter::format const and keep this format function const.

Ok, by in my opinion, there is a potential problem with this solution, with making format() function in formatters const - maybe if a some of formatters have format() function non-const on purpose then it cannot be used at compile-time. I just don't know:

why all or almost all formatters currently defined in {fmt} has non-const format() function?

how rare it is that format() function in custom formatters is non-const on purpose?

I mean, I can definitely try to make this function const for formatters which are used in test, but since you have better perspective here it will be helpful if you agree or disagree with me for this potential problem.

Actually, I don't even consider to go this way because some already created formatters would be unavailable at compile-time. On the other hand the code I have in this PR right now does not change formatters at all.

why all or almost all formatters currently defined in {fmt} has non-const format() function?

No good reason, just less typing =).

how rare it is that format() function in custom formatters is non-const on purpose?

I don't think it should necessary at all. format shouldn't mutate formatter, only apply format specifiers stored in it to the input (or rather output =)).

already created formatters would be unavailable at compile-time

We can fix this for all {fmt} formatters and later provide a workaround for user-defined ones.

Ok, I reverted back most of the changes in compile.h and mark all of formatters as const, at least those I found.

and later provide a workaround for user-defined ones.

But there is no such thing as workarounds at compile-time 🙂, that why it's so cool to check your code just by making it execute at compile-time. Of course, I'm talking about workarounds that are for removing the constness of objects, not interface workarounds.

The workaround is to copy the formatter when needed.

include/fmt/compile.h

include/fmt/core.h

vitaut · 2020-12-12T16:02:07Z

include/fmt/core.h

-  FMT_INLINE basic_format_args(const format_arg_store<Context, Args...>& store)
+  constexpr basic_format_args(const format_arg_store<Context, Args...>& store)


Here and elsewhere: please keep FMT_INLINE.

Yes, I missed that FMT_INLINE has an attribute inside, and this line can use FMT_INLINE and constexpr. But those lines where FMT_INLINE was replaced by FMT_CONSTEXPR(20) will have double inline specifier, which makes some compilers unhappy and some very unhappy.

So looks like one of these macros shouldn't set inline and that would be really strange if this macro is going to be FMT_INLINE.

I can give an example of Hedley, it does not fallback to inline in the case where constexpr is not available - https://github.com/nemequ/hedley/blob/5e50f6b735aeb4e09e3b2ad3d1717db3c0613bfa/hedley.h#L1269-L1276
and I think it's right because I can always mark a function as inline with or without constexpr, no matter that constexpr implies inline.

In case you agree with me here I can create a separate PR to remove inline from FMT_CONSTEXPR(20) macro. Otherwise I just don't know how to fix these problems without introducing FMT_CONSTEXPR(20)_NO_INLINE macro, which probably would be also bad naming because there is FMT_NOINLINE macro that expands to additional attribute...

In case you agree with me here I can create a separate PR to remove inline from FMT_CONSTEXPR(20) macro.

Sure.

vitaut · 2020-12-20T15:04:47Z

I suggest adding define for this whole formatting at compile-time feature in the same manner as with other features, so if it's not predefined then set it by the expression we already have __cplusplus >= 202002L. Is it a good idea, should I do that in this PR or not?

We shouldn't be adding a macro for this. Instead __cplusplus >= 202002L should be replaced with a more specific check.

vitaut · 2020-12-20T15:08:07Z

include/fmt/compile.h

-  OutputIt format(OutputIt out, const Args&... args) const {
+  constexpr OutputIt format(OutputIt out, const Args&... args) {


I think the correct solution is to make formatter::format const and keep this format function const.

include/fmt/core.h

for `is_constant_evaluated()` function

to revert back some changes in compile.h

vitaut · 2020-12-23T14:32:10Z

include/fmt/format.h

+    const char* digits = upper ? "0123456789ABCDEF"
+                               : (is_constant_evaluated() ? "0123456789abcdef"
+                                                          : data::hex_digits);


Can we make data::hex_digits constexpr instead?

But I tried to make them available both at compile-time and at runtime via macro definitions. Right now they are embedding into the static library, so I don't even touch them in this PR to not break the ABI. Maybe such variables should be declared as constexpr and be copied to that static data structure for runtime uses and still be chosen for compile-time uses.

Also, we can go the same way as with digits - by implementing cast specially for compile-time. It's going to be more complex, but it's possible.

I mean defining it as

static constexpr char hex_digits[] = "0123456789abcdef";

in the basic_data struct. This is a tiny piece of data so we don't need to worry about having it in the header.

vitaut · 2020-12-23T14:33:04Z

include/fmt/format.h

+    auto* shifts = align == align::left ? data::left_padding_shifts
+                                        : data::right_padding_shifts;


Same with shifts.

vitaut · 2020-12-23T14:49:13Z

include/fmt/format.h

+  }
+
+  template <typename FormatContext>
+  FMT_CONSTEXPR auto format(const T& val, FormatContext& ctx)


non-const overload is not needed, please remove.

The const one copies the specs_ member into a temporary variable to be able to modify it, while the non-const one just uses this member field. So this change can introduce performance loss if this copy is heavy enough.

That's fine, we can optimize this later.

for functions where `hex_digits` or shifts from `basic_data` is used

vitaut · 2020-12-25T14:40:09Z

Thank you!

alexezeder · 2020-12-25T15:19:11Z

But there is still the problem with FMT_INLINE...

vitaut · 2020-12-25T15:38:02Z

But there is still the problem with FMT_INLINE.

I thought you are addressing this in a separate PR.

alexezeder · 2020-12-25T15:42:07Z

Yes, but in this PR some of them is missing due to a need for FMT_CONSTEXPR, but I will probably revert them back in that separate PR.
I just hoped that that PR would be merged before this 🙂.

alexezeder added 4 commits December 9, 2020 02:58

add tests for specs, std::string_view -> fmt::basic_string_view in te…

725eedf

…st_string

add constexpr where it's needed

aead58e

add missing 0 spec test

62abf10

quick fix for MSVC 19.0.24241.7

6742642

vitaut requested changes Dec 12, 2020

View reviewed changes

use is_constant_evaluated() to initialize class value` with string

de42f13

alexezeder requested a review from vitaut December 15, 2020 02:06

vitaut requested changes Dec 20, 2020

View reviewed changes

alexezeder added 4 commits December 22, 2020 21:28

use FMT_NOEXCEPT instead of FMT_DETECTED_NOEXCEPT

fec7c1a

for `is_constant_evaluated()` function

remove braces for if in value constuctor

2980dd3

mark formatter<>::format() functions as const

b8d973a

to revert back some changes in compile.h

enable formatting at compile-time for GCC 10.2+

6714f90

alexezeder requested a review from vitaut December 22, 2020 20:15

vitaut requested changes Dec 23, 2020

View reviewed changes

alexezeder mentioned this pull request Dec 24, 2020

Remove fallback to inline specifier from FMT_CONSTEXPR(20) macro #2075

Merged

alexezeder added 5 commits December 25, 2020 03:07

fix macro check for formatting at compile-time on GCC 10.2+

8131d4f

make hex_digits and shifts in basic_data constexpr

14ce33e

remove non-const format method from formatter

4762f64

add missing constexpr in definitions of static members

5cca27f

replace FMT_CONSTEXPR20 with FMT_CONSTEXPR

1b1c3ce

for functions where `hex_digits` or shifts from `basic_data` is used

alexezeder requested a review from vitaut December 25, 2020 00:36

vitaut merged commit bbd6ed5 into fmtlib:master Dec 25, 2020

alexezeder added a commit to alexezeder/fmt that referenced this pull request Dec 27, 2020

revert back FMT_INLINE removed in fmtlib#2056

f5e9b86

alexezeder deleted the feature/formatting_at_compile_time_specs branch January 15, 2021 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support of most format_specs for formatting at compile-time #2056

Add support of most format_specs for formatting at compile-time #2056

alexezeder commented Dec 9, 2020 •

edited

Loading

alexezeder commented Dec 10, 2020 •

edited

Loading

vitaut left a comment

vitaut Dec 12, 2020

alexezeder Dec 15, 2020

vitaut Dec 20, 2020

alexezeder Dec 20, 2020

vitaut Dec 20, 2020 •

edited

Loading

alexezeder Dec 22, 2020 •

edited

Loading

vitaut Dec 25, 2020

vitaut Dec 12, 2020

alexezeder Dec 15, 2020

alexezeder Dec 22, 2020

vitaut Dec 23, 2020

alexezeder Dec 24, 2020

vitaut commented Dec 20, 2020

vitaut Dec 20, 2020

vitaut Dec 23, 2020

alexezeder Dec 23, 2020

alexezeder Dec 23, 2020

vitaut Dec 23, 2020

vitaut Dec 23, 2020

vitaut Dec 23, 2020

alexezeder Dec 23, 2020 •

edited

Loading

vitaut Dec 23, 2020

vitaut commented Dec 25, 2020

alexezeder commented Dec 25, 2020

vitaut commented Dec 25, 2020

alexezeder commented Dec 25, 2020 •

edited

Loading

		OutputIt format(OutputIt out, const Args&... args) const {
		constexpr OutputIt format(OutputIt out, const Args&... args) {

		FMT_INLINE basic_format_args(const format_arg_store<Context, Args...>& store)
		constexpr basic_format_args(const format_arg_store<Context, Args...>& store)

		auto* shifts = align == align::left ? data::left_padding_shifts
		: data::right_padding_shifts;

Add support of most format_specs for formatting at compile-time #2056

Add support of most format_specs for formatting at compile-time #2056

Conversation

alexezeder commented Dec 9, 2020 • edited Loading

alexezeder commented Dec 10, 2020 • edited Loading

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

vitaut Dec 20, 2020 • edited Loading

Choose a reason for hiding this comment

alexezeder Dec 22, 2020 • edited Loading

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 commented Dec 20, 2020

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

alexezeder Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaut commented Dec 25, 2020

alexezeder commented Dec 25, 2020

vitaut commented Dec 25, 2020

alexezeder commented Dec 25, 2020 • edited Loading

alexezeder commented Dec 9, 2020 •

edited

Loading

alexezeder commented Dec 10, 2020 •

edited

Loading

vitaut Dec 20, 2020 •

edited

Loading

alexezeder Dec 22, 2020 •

edited

Loading

alexezeder Dec 23, 2020 •

edited

Loading

alexezeder commented Dec 25, 2020 •

edited

Loading