-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow format_to_n to be executed at compile time #4365
Conversation
8e175bf
to
81b2930
Compare
After the first failed workflow run fixed the condition when the test runs and remove WIP change I forgot about |
…ffer move constructor to fix deprectaion warning on clang-9
I tried playing around with this a little bit. At least under VS17.14, this doesn't work for more complex format strings. For example, fmt::format_to_n(buffer.data(), buffer.size(), "{}", 4); // Your test case, totally fine.
fmt::format_to_n(buffer.data(), buffer.size(), "{}{}", 4, 2); // Fails to compile due to an expression not being a constant.
fmt::format_to_n(buffer.data(), buffer.size(), "{:d}", 42); // Fails to compile due to an expression not being a constant. I suppose that's why in your test, you do multiple calls to I would say it reduces how interesting this is, it only allows a format string of |
// This test doesn't have to be extensive - | ||
// it just checks format_to_n can be done in constexpr context | ||
constexpr bool result = []{ | ||
std::array buffer {'x', 'x', 'x', 'x'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows compilation is failing because it needs #include <array>
for this.
@@ -1835,6 +1835,7 @@ class fixed_buffer_traits { | |||
|
|||
public: | |||
constexpr explicit fixed_buffer_traits(size_t limit) : limit_(limit) {} | |||
FMT_CONSTEXPR20 ~fixed_buffer_traits() = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why FMT_CONSTEXPR20 and not FMT_CONSTEXPR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that destructors cannot be marked constexpr before C++ 20
@@ -2392,8 +2397,8 @@ FMT_CONSTEXPR inline auto is_locking() -> bool { | |||
return locking<T1>::value || is_locking<T2, Tail...>(); | |||
} | |||
|
|||
FMT_API void vformat_to(buffer<char>& buf, string_view fmt, format_args args, | |||
locale_ref loc = {}); | |||
FMT_API FMT_CONSTEXPR void vformat_to(buffer<char>& buf, string_view fmt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be constexpr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NVM, I thought you are trying to make a compiled version of format_to_n
constexpr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this forward declaration confused me at first and I thought an addition macro will be necessary (FMT_CONSTEXPR_IF_HEADERONLY
😄 ) but it is defined in the same header.
@@ -475,3 +475,18 @@ TEST(format_impl_test, to_utf8) { | |||
EXPECT_EQ(s, u.str()); | |||
EXPECT_EQ(s.size(), u.size()); | |||
} | |||
|
|||
#if FMT_USE_CONSTEVAL | |||
TEST(format_test, format_to_n_constexpr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go to compile-test.
Thanks for the PR but there is already a compile-time version of Lines 497 to 498 in 577fd3b
constexpr (at least not yet).
|
Well, I didn't avoid complex format strings intentionally - just wanted to make it work and see your suggestions about what cases should be working to make it decent or a reason why you don't want/need it.
I am not sure I follow which version you mean. I tried a few different ways (like this to get any formatting happening at compile time with no success. So I would appreciate any suggestions 🙏. Thank you for your review! |
@Sunday111 Read up on FMT_COMPILE and see if it addresses what you need. That's the version @vitaut is talking about. fmt::format_to_n(array, N, FMT_COMPILE("{}"), 42); // An example @vitaut I think what they're after is an ability to generate the full, rendered string at compile time, not just compile the format string and then render at runtime. I believe the I suppose the question should be, is the feature for compile time rendering interesting for {fmt} and should it work towards supporting it? I can't answer that question, I just try to contribute. 😉 This is pure conjecture, because I haven't done any perf checks, but I imagine the expensive part of {fmt} is the format parsing and not the value insertion (ignoring custom types). @Sunday111 maybe that's what @vitaut is getting at. You likely don't need compile time rendering as you can just use I still think it's a fun idea and amazed at how little it took to get it to work. 😄 |
Looks like |
@vitaut I see. Yes, it is not constexpr, and constexpr implementation of back_inserter would be required to make it constexpr. With So I have to write an iterator either way, it seems. |
|
Hello!
Currently
fmt::format_to_n
cannot be executed at compile time. I needed this functionality in one of my personal project and didn't see a reason why wouldn't it be possible so here is a PR for that.