Always use FMT_STRING internally where possible [Issue #2002] by yeswalrus · Pull Request #2006 · fmtlib/fmt

yeswalrus · 2020-11-11T00:01:50Z

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.

vitaut · 2020-11-12T13:56:02Z

 template <typename S, typename... Args, typename Char = char_t<S>>
 inline std::basic_string<Char> format(const text_style& ts, const S& format_str,
-                                      const Args&... args) {
+                                      Args&&... args) {


This looks unrelated, please revert.

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

vitaut

Thanks for the PR. Looks good in general but please address the inline comment.

vitaut · 2020-11-13T13:36:08Z

  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));
+  }


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.

ah, good to know! Won't that break on platforms where FMT_CONSTEXPR_DECL evaluates to noop though?

On that platform FMT_STRING should also evaluate to noop (at least in theory).

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 · 2020-11-14T01:42:32Z

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
I've modified chrono.h to use enable_if in a sadly combinatoric way to avoid having to use FMT_STRING on a constexpr char array and to avoid compiling a compile-time checked format string with a bad type. LMK if you are aware of a better solution for this.

yeswalrus · 2020-11-14T02:24:58Z

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.

vitaut

Mostly looks good but please address inline comments.

vitaut · 2020-11-14T13:57:36Z

-  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);


Please apply clang-format to your changes.

vitaut · 2020-11-14T13:59:57Z

+  (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


I don't think we need this because we already test this below.

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.

vitaut · 2020-11-15T13:19:13Z

Thank you!

Always use FMT_STRING internally where possible

9501da5

yeswalrus mentioned this pull request Nov 11, 2020

FMT_ENFORCE_COMPILE_STRING broken in 7.1.2 with format-inl.h #2002

Closed

fixed chrono.h changes

854bf9a

yeswalrus changed the title ~~Always use FMT_STRING internally where possible [#2002]~~ Always use FMT_STRING internally where possible [Issue #2002] Nov 11, 2020

fixup type errors

67a4447

vitaut reviewed Nov 12, 2020

View reviewed changes

yeswalrus requested a review from vitaut November 12, 2020 22:24

revert removal of const

969dcce

yeswalrus force-pushed the fix-forced-compile-checks branch from d4742be to 969dcce Compare November 12, 2020 22:26

vitaut requested changes Nov 13, 2020

View reviewed changes

use constexpr

45c7c29

yeswalrus requested a review from vitaut November 13, 2020 19:41

Walter Gray added 3 commits November 13, 2020 11:45

use FMT_CONSTEXPR_DECL

eefa1e5

use FMT_ENABLE_IF for compile-time if branching

93f1e22

ignore unusable param in non-float overload

03c29de

yeswalrus force-pushed the fix-forced-compile-checks branch from af0ede3 to 03c29de Compare November 14, 2020 00:17

Walter Gray added 2 commits November 13, 2020 17:16

workaround MSVC bug, add test

2c271ac

#ifdef test broken in MSVC

45b9f96

Walter Gray added 2 commits November 13, 2020 17:47

improve test, fix conditional

98a3557

attempt simplification to work around MSVC problem

fc3832f

yeswalrus force-pushed the fix-forced-compile-checks branch from 07f3550 to fc3832f Compare November 14, 2020 02:21

vitaut requested changes Nov 14, 2020

View reviewed changes

clang-format

d5e3e34

yeswalrus requested a review from vitaut November 14, 2020 22:08

vitaut merged commit befd7d4 into fmtlib:master Nov 15, 2020

yeswalrus deleted the fix-forced-compile-checks branch November 15, 2020 23:04

Uh oh!

Uh oh!

Conversation

yeswalrus commented Nov 11, 2020

Uh oh!

vitaut Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

yeswalrus Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

vitaut left a comment

Choose a reason for hiding this comment

Uh oh!

vitaut Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yeswalrus Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

vitaut Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

yeswalrus Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

yeswalrus commented Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yeswalrus commented Nov 14, 2020

Uh oh!

vitaut left a comment

Choose a reason for hiding this comment

Uh oh!

vitaut Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

vitaut Nov 14, 2020

Choose a reason for hiding this comment

Uh oh!

yeswalrus Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vitaut commented Nov 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vitaut Nov 13, 2020 •

edited

Loading

yeswalrus commented Nov 14, 2020 •

edited

Loading

yeswalrus Nov 14, 2020 •

edited

Loading