Always use FMT_STRING internally where possible [Issue #2002] #2006
Conversation
yeswalrus
changed the title
include/fmt/color.h
Outdated
| const Args&... args) { | ||
| Args&&... args) { |
There was a problem hiding this comment.
This looks unrelated, please revert.
There was a problem hiding this comment.
Done, though this was done since I noticed it would bring the function more into alignment with the other examples of and documentation for functions using make_args_checked
yeswalrus
force-pushed
the
fix-forced-compile-checks
branch
from
d4742be
to
969dcce
Compare
include/fmt/chrono.h
Outdated
| const Char pr_f[] = {'{', ':', '.', '{', '}', 'f', '}', 0}; | ||
| if (precision >= 0) return format_to(out, pr_f, val, precision); | ||
| if (precision >= 0) { | ||
| return vformat_to(out, to_string_view(pr_f), | ||
| make_format_args<context>(val, precision)); | ||
| } |
There was a problem hiding this comment.
Please make pr_f static constexpr and keep format_to (don't change it to vformat_to). static constexpr strings should work with FMT_STRING:
static constexpr const Char pr_f[] = {'{', ':', '.', '{', '}', 'f', '}', 0};
if (precision >= 0) return format_to(out, FMT_STRING(pr_f), val, precision);Same below.
There was a problem hiding this comment.
ah, good to know! Won't that break on platforms where FMT_CONSTEXPR_DECL evaluates to noop though?
There was a problem hiding this comment.
On that platform FMT_STRING should also evaluate to noop (at least in theory).
There was a problem hiding this comment.
This apparently breaks on windows, even after correcting it to use SFINAE to avoid compiling the {:.{}f} format string with an integral type. I'm adding a test for using FMT_STRING with a static FMT_CONSTEXPR_DECL, and I'll see if I can get a workaround
yeswalrus
force-pushed
the
fix-forced-compile-checks
branch
from
af0ede3
to
03c29de
Compare
|
I know you asked me not to modifiy tests, but I've added a demonstration of a failure to format-test.cc starting at line 1819, and filed a bug report against MSVC++ here: https://developercommunity.visualstudio.com/content/problem/1256077/internal-compiler-failure-with-macro-defining-lamb.html |
yeswalrus
force-pushed
the
fix-forced-compile-checks
branch
from
07f3550
to
fc3832f
Compare
|
I've simplified by directly calling the underlying function, detail::compile_string_to_view, to work around the MSVC crash. It simplifies things significantly, so LMK which version you'd prefer. |
include/fmt/chrono.h
Outdated
| return format_to(out, std::is_floating_point<Rep>::value ? fp_f : format, | ||
| val); | ||
| static FMT_CONSTEXPR_DECL const Char pr_f[] = {'{', ':', '.', '{', '}', 'f', '}', 0}; | ||
| if (precision >= 0) return format_to(out, compile_string_to_view(pr_f), val, precision); |
There was a problem hiding this comment.
Please apply clang-format to your changes.
| (void)static_with_null; | ||
| (void)static_with_null_wide; | ||
| (void)static_no_null; | ||
| (void)static_no_null_wide; | ||
| #if !defined(_MSC_VER) | ||
| EXPECT_EQ("42", fmt::format(FMT_STRING(static_with_null), 42)); | ||
| EXPECT_EQ(L"42", fmt::format(FMT_STRING(static_with_null_wide), 42)); | ||
| EXPECT_EQ("42", fmt::format(FMT_STRING(static_no_null), 42)); | ||
| EXPECT_EQ(L"42", fmt::format(FMT_STRING(static_no_null_wide), 42)); | ||
| #endif |
There was a problem hiding this comment.
I don't think we need this because we already test this below.
There was a problem hiding this comment.
Not quite - you test with constexpr locals, which are a c++ 17(14?) feature, but static constexpr char arrays are valid all the way back to c++11 and this exposes a bug in MSVC.
|
Thank you! |
Update a few internal calls to fmt::format to use FMT_STRING, improving compatibility with the FMT_ENFORCE_COMPILE_STRING compile option.
I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.