Add tests for FMT_ENFORCE_COMPILE_STRING, fix several errors #2038

yeswalrus · 2020-11-30T19:12:55Z

Though all format strings will be compile time checked by default in c++20, FMT_ENFORCE_COMPILE_STRING will still be useful for users stuck on older c++ versions.

This PR adds a test file with API coverage taken from other existing tests, uncovering 4 places where FMT_ENFORCE_COMPILE_STRING caused unexpected breakages. It additionally resolves those errors, allowing the test to pass.

yeswalrus · 2020-12-01T18:38:58Z

include/fmt/chrono.h

-  return format_to(out, compile_string_to_view(format), val);
+
+// Workaround a compiler error in MSVC < 16.8.2
+#if defined(FMT_MSC_VER) && FMT_MSC_VER <= 1928


My last fix was actually in error and did not hold up to CI once a test was in place. Microsoft states that this is fixed as of 16.8.2 however I cannot verify since the most recent version on godbolt is 19.27

yeswalrus · 2020-12-01T18:39:34Z

include/fmt/core.h

@@ -1841,6 +1841,15 @@ inline auto format_to_n(OutputIt out, size_t n, const S& format_str,
  Returns the number of characters in the output of
  ``format(format_str, args...)``.
 */
+template <typename S, typename... Args,
+          FMT_ENABLE_IF(is_compile_string<S>::value)>
+inline size_t formatted_size(const S& format_str, Args&&... args) {


This appeared to be a gap in the API

This looks unrelated to the current PR.

I could break it out into a separate followup PR, but it seemed relevant since without it there's no way to call formatted_size while FMT_ENFORCE_COMPILE_STRING is set

Please move to a separate PR.

vitaut

Thanks for the PR.

Testing FMT_ENFORCE_COMPILE_STRING is a good idea but I think we should avoid duplicating tests. The new test should only check that the public headers compile with FMT_ENFORCE_COMPILE_STRING set with no or few checks of its own.

vitaut · 2020-12-03T19:05:09Z

include/fmt/chrono.h

+// Workaround a compiler error in MSVC <= 16.8.2
+#if FMT_MSC_VER && FMT_MSC_VER <= 1928
+  return vformat_to(out, to_string_view(format),
+                    make_format_args<buffer_context<Char>>(val));
+#else
+  return format_to(out, FMT_STRING(format), val);
+#endif


Could you move this workaround to a separate PR and provide more details?

yeswalrus · 2020-12-03T20:22:17Z

I agree we should avoid duplicating tests, but we kinda have to to a certain extent since without a file exercising the APIs nothing will actually get instantiated? One option might be making a separate file for all the tests of FMT_STRING specifically that could then be compiled with and without FMT_ENFORCE_COMPILE_STRING, would that work for you?

vitaut · 2020-12-04T17:15:38Z

I agree we should avoid duplicating tests, but we kinda have to to a certain extent since without a file exercising the APIs nothing will actually get instantiated?

Sure but we don't need to replicate the tests, only force the necessary instantiations. Therefore I suggest removing all assertions to make it clear that this is not testing formatting functionality (which should be tested elsewhere) and only leave function calls that trigger instantiations.

vitaut · 2020-12-05T14:39:05Z

doc/api.rst

+Enforcing Compile-time Format Checks
+------------------------------------


I think this doesn't need to be a separate section. Instead add a paragraph to the Compile-time Format String Checks section above.

vitaut · 2020-12-05T14:39:37Z

doc/api.rst

@@ -165,6 +165,14 @@ functions in their ``formatter`` specializations.

 .. _udt:

+Enforcing Compile-time Format Checks
+------------------------------------
+To force the use of Compile-time checks, compile with 


Compile -> compile

yeswalrus · 2020-12-09T19:30:28Z

#2049 was closed, but the modifications to chrono.h are required. An alternative here might be to say that FMT_ENFORCE_COMPILE_STRING is simply not supported at all in MSVC, and disable the enforce_compile_string test for that compiler

vitaut · 2020-12-12T15:32:35Z

include/fmt/chrono.h

+// Note(12/3/2020): Workaround an as-of-yet unfixed compiler error in MSVC.
+// See https://developercommunity.visualstudio.com/content/problem/1277597/internal-compiler-c0001-error-on-complex-nested-la.html
+#if FMT_MSC_VER
+  return vformat_to(out, to_string_view(format),
+                    make_format_args<buffer_context<Char>>(val));
+#else
+  return format_to(out, FMT_STRING(format), val);
+#endif


As commented in #2049, I don't think it's worth doing.

vitaut · 2020-12-12T15:35:12Z

test/CMakeLists.txt

@@ -106,6 +106,10 @@ add_fmt_test(printf-test)
 add_fmt_test(ranges-test)
 add_fmt_test(scan-test)

+


nit: extra newline

vitaut · 2020-12-12T15:36:03Z

test/CMakeLists.txt

@@ -106,6 +106,10 @@ add_fmt_test(printf-test)
 add_fmt_test(ranges-test)
 add_fmt_test(scan-test)

+
+add_fmt_test(enforce-compiletime-test)


enforce-compiletime-test -> enforce-compile-string-test for consistency with the macro name

vitaut · 2020-12-12T15:36:22Z

test/CMakeLists.txt

@@ -106,6 +106,10 @@ add_fmt_test(printf-test)
 add_fmt_test(ranges-test)
 add_fmt_test(scan-test)

+
+add_fmt_test(enforce-compiletime-test)
+target_compile_definitions(enforce-compiletime-test PRIVATE "-DFMT_ENFORCE_COMPILE_STRING")


Please break the long line.

vitaut · 2020-12-12T15:37:26Z

test/enforce-compiletime-test.cc

+#ifdef WIN32
+#  define _CRT_SECURE_NO_WARNINGS
+#endif


This is not needed, please remove.

vitaut · 2020-12-12T15:42:10Z

test/enforce-compiletime-test.cc

+struct test_output_iterator {
+  char* data;
+
+  using iterator_category = std::output_iterator_tag;
+  using value_type = void;
+  using difference_type = void;
+  using pointer = void;
+  using reference = void;
+
+  test_output_iterator& operator++() {
+    ++data;
+    return *this;
+  }
+  test_output_iterator operator++(int) {
+    auto tmp = *this;
+    ++data;
+    return tmp;
+  }
+  char& operator*() { return *data; }
+};
+
+void FormatToNOutputIteratorTest() {
+  char buf[10] = {};
+  fmt::format_to_n(test_output_iterator{buf}, 10, FMT_STRING("{}"), 42);
+}


This is redundant, please remove.

vitaut · 2020-12-12T15:42:28Z

test/enforce-compiletime-test.cc

+}
+
+void TestChrono() {
+#ifndef FMT_STATIC_THOUSANDS_SEPARATOR


Why conditionally compiled?

TBH, I'm not sure - all the tests covering formatting of chrono types in chrono_test.cc were too so I figured it was important? I'll try removing it

I lack context for this particular test, but: FMT_STATIC_THOUSANDS_SEPARATOR is "overloaded" to mean "I don't like <locale> and want nothing to do with it", and so the locale_ref functions are not implemented if FMT_STATIC_THOUSANDS_SEPARATOR is defined. However, chrono_formatter::format_localized always tries to use the locale_ref functions, so chrono-test won't link in that situation.

Thank you for the explanation. It seems to work just fine without the ifdef in this situation so I'll leave them removed.

vitaut · 2020-12-12T15:43:22Z

test/enforce-compiletime-test.cc

+struct zstring_sentinel {};
+
+bool operator==(const char* p, zstring_sentinel) { return *p == '\0'; }
+bool operator!=(const char* p, zstring_sentinel) { return *p != '\0'; }
+
+struct zstring {
+  const char* p;
+  const char* begin() const { return p; }
+  zstring_sentinel end() const { return {}; }
+};
+
+void TestZString() {
+  zstring hello{"hello"};
+  (void)fmt::format(FMT_STRING("{}"), hello);
+  (void)fmt::format(FMT_STRING("{}"), fmt::join(hello, "_"));
+}


This is redundant.

I removed the 2nd overload but this actually exposed an issue with ranges.h:294

vitaut · 2020-12-12T15:43:35Z

test/enforce-compiletime-test.cc

+  (void)fmt::format(FMT_STRING("{}"), fmt::join(hello, "_"));
+}
+
+int main(int, char**) {


Arguments are unused, please remove.

vitaut · 2020-12-12T15:43:43Z

test/enforce-compiletime-test.cc

+
+  return 0;


This is not needed.

yeswalrus · 2020-12-13T02:07:01Z

I've tried to solve the issue with GCC & calls to format with no arguments, but I'm not super familiar with the constexpr machinery at play & how to debug it. All the fixes I've tried so far also allow fmt::format(FMT_STRING("{}")) to compile, which it definitely shouldn't

vitaut · 2020-12-19T17:29:01Z

include/fmt/chrono.h

@@ -769,18 +769,24 @@ template <typename Char, typename Rep, typename OutputIt,
          FMT_ENABLE_IF(std::is_integral<Rep>::value)>
 OutputIt format_duration_value(OutputIt out, Rep val, int) {
  static FMT_CONSTEXPR_DECL const Char format[] = {'{', '}', 0};
-  return format_to(out, compile_string_to_view(format), val);
+


nit: extra newline

vitaut · 2020-12-19T17:29:36Z

include/fmt/chrono.h

@@ -769,18 +769,24 @@ template <typename Char, typename Rep, typename OutputIt,
          FMT_ENABLE_IF(std::is_integral<Rep>::value)>
 OutputIt format_duration_value(OutputIt out, Rep val, int) {
  static FMT_CONSTEXPR_DECL const Char format[] = {'{', '}', 0};
-  return format_to(out, compile_string_to_view(format), val);
+
+  return vformat_to(out, to_string_view(format),


Here and below: why not wrap format in FMT_STRING and use format_to?

Because then it wouldn't compile on windows because of the MSVC error? Thats why I was submitting that bug report and had that here before

AFAICS the MSVC bug is specific to static constexpr which we don't need here. I suggest replacing with:

return format_to(out, FMT_STRING("{}"), val);

which seems to work: https://godbolt.org/z/Teqnhq.

I thought we needed the static constexpr array so that the format string type would match the type of Char?

On a second thought, we don't need format_to at all. This should be just write<Char>(out, val).

Excellent! I had no idea about that API. I've redone the other calls that were using format_to in chrono as well.

vitaut · 2020-12-19T17:30:49Z

include/fmt/core.h

@@ -1841,6 +1841,7 @@ inline auto format_to_n(OutputIt out, size_t n, const S& format_str,
  Returns the number of characters in the output of
  ``format(format_str, args...)``.
 */
+


Extra newline

vitaut · 2020-12-19T17:32:45Z

include/fmt/ranges.h

-      out = format_to(out,
-                      detail::format_str_quoted(
-                          (formatting.add_delimiter_spaces && i > 0), v),
-                      v);
+      out = vformat_to(out,
+                       detail::format_str_quoted(
+                           (formatting.add_delimiter_spaces && i > 0), v),
+                       make_format_args<FormatContext>(v));


Here and below: it would be better to refactor this to use compile string and format_to.

I considered that but given that the format string used appeared to be determined at run-time I wasn't sure it was possible. I'll give it a go.

Looking at this more I'm reaching the same conclusion - formatting can't be used from within FMT_STRING - doing so causes

/home/walter.gray/development/walrus/fmt/include/fmt/ranges.h:371:29: error: use of non-static data member 'formatting' of 'formatter<T, Char, enable_if_t<fmt::is_range<T, Char>::value && (has_formatter<detail::value_type<T>, format_context>::value || detail::has_fallback_formatter<detail::value_type<T>, format_context>::value)> >' from nested type 'FMT_COMPILE_STRING' (formatting.add_delimiter_spaces && i > 0), *it)),

I couldn't have a function that returned one of several format strings, and I don't think I could have a function that returned a constexpr string that would work with FMT_STRING either -

constexpr const char* getString() { return "{} bananna"; } int main(int argc, char** argv) { fmt::print(FMT_STRING(getString()), "fried"); }

for example will not compile in godbolt. I also cant return the result of FMT_STRING since they'd be different types between branches. My best guess for how to do this would be to have a wrapping function that used template specialization on a bool for add_delimiter_spaces and runtime switching on i > 0 and called format_to with the appropriate FMT_STRING directly. Am I missing an option here or should I try that?

Let's use write instead of format_to here too.

vitaut · 2020-12-19T17:33:06Z

test/CMakeLists.txt

@@ -106,6 +106,14 @@ add_fmt_test(printf-test)
 add_fmt_test(ranges-test)
 add_fmt_test(scan-test)

+if(NOT MSVC)


nit: missing space after if.

vitaut · 2020-12-19T17:33:44Z

test/enforce-compile-string-test.cc

+#include <array>
+#include <iterator>
+#include <list>
+#include <map>
+#include <sstream>
+#include <string>
+#include <utility>
+#include <vector>


Some of these are unused.

vitaut · 2020-12-19T17:34:05Z

test/enforce-compile-string-test.cc

+
+// Exercise the API to verify that everything we expect to can compile.
+void test_format_api() {
+  (void)fmt::format(FMT_STRING("{}"), 42);


I don't think cast is necessary.

IIRC apple clang complains about an unused return value if it's not present, I'll try it again though.

vitaut · 2020-12-19T17:36:50Z

test/enforce-compile-string-test.cc

+#if !FMT_GCC_VERSION  // Currently will not compile: See
+                      // https://github.com/fmtlib/fmt/issues/2039


This should compile now, please rebase and remove.

vitaut · 2020-12-19T17:37:52Z

test/enforce-compile-string-test.cc

+  (void)udl_format;
+  (void)udl_format_w;


Variables are not needed, please remove.

vitaut · 2020-12-19T17:39:10Z

test/enforce-compile-string-test.cc

+struct zstring_sentinel {};
+
+bool operator==(const char* p, zstring_sentinel) { return *p == '\0'; }
+bool operator!=(const char* p, zstring_sentinel) { return *p != '\0'; }
+
+struct zstring {
+  const char* p;
+  const char* begin() const { return p; }
+  zstring_sentinel end() const { return {}; }
+};
+
+void test_zstring() {
+  zstring hello{"hello"};
+  (void)fmt::format(FMT_STRING("{}"), hello);
+}


This looks unnecessary. If you want to check ranges, you can use a standard container.

vitaut

Mostly looks good, just a few minor comments.

vitaut · 2020-12-21T16:28:34Z

include/fmt/chrono.h

+  basic_memory_buffer<Char> buffer;
+  auto bufOut = std::back_inserter(buffer);


This is unnecessary. You can write to out directly.

vitaut · 2020-12-21T16:30:56Z

include/fmt/ranges.h

-      out = format_to(out,
-                      detail::format_str_quoted(
-                          (formatting.add_delimiter_spaces && i > 0), v),
-                      v);
+      out = vformat_to(out,
+                       detail::format_str_quoted(
+                           (formatting.add_delimiter_spaces && i > 0), v),
+                       make_format_args<FormatContext>(v));


Let's use write instead of format_to here too.

vitaut · 2020-12-21T16:32:07Z

test/enforce-compile-string-test.cc

+}
+
+void test_range() {
+  std::array<char, 5> hello = {'h','e','l','l','o'};


Please apply clang-format.

yeswalrus · 2020-12-21T19:18:40Z

Done! Good call on refactoring to use write - worked like a charm and IMO the code is easier to follow now as well.

vitaut · 2020-12-23T14:12:17Z

include/fmt/locale.h

-inline auto format_to(OutputIt out, const std::locale& loc,
-                      const S& format_str, Args&&... args) ->
+inline auto format_to(OutputIt out, const std::locale& loc, const S& format_str,
+                      Args&&... args) ->


Unrelated, please revert.

vitaut · 2020-12-23T14:18:07Z

include/fmt/ranges.h

+OutputIt add_range_formatting_spaces(OutputIt out,
+                                     const Formatting& formatting) {


add_range_formatting_spaces -> write_delimiter

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

include/fmt/ranges.h

+  if (const_check(formatting.add_prepostfix_space)) {
+    *out++ = ' ';
+  }


Let's just remove this since add_prepostfix_space is always false.

My impression was that in user-declared overloads it could not be? (nm, I see thats impossible)

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

include/fmt/ranges.h

+  if (const_check(formatting.add_delimiter_spaces)) {
+    *out++ = ' ';
+  }


Let's write space unconditionally since add_delimiter_spaces is always true and remove formatting argument.

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

include/fmt/ranges.h

-  return add_space ? " \"{}\"" : "\"{}\"";
+template <
+    typename Char, typename OutputIt, typename Arg,
+    FMT_ENABLE_IF(std::is_same<Arg, const char*>::value ||


What is std::is_same<Arg, const char*>::value for?

I thought that this was required to make it so that for example a const char * array_of_literals[] would be formatted with "s, but I appear to be incorrect. I've added a test-case to cover that and you're correct, it appears unnecessary, though I couldn't say why.

vitaut · 2020-12-23T14:23:18Z

include/fmt/ranges.h

-    if (formatting.add_prepostfix_space) *out++ = ' ';
+    if (formatting.add_prepostfix_space) {
+      *out++ = ' ';
+    }


Please revert (or remove).

vitaut · 2020-12-24T14:40:55Z

Thank you!

yeswalrus force-pushed the test_enforce_compile_string branch 5 times, most recently from 1b97d1d to 930c554 Compare November 30, 2020 22:35

yeswalrus mentioned this pull request Dec 1, 2020

fmt::format/print(FMT_STRING("noarg string")) is broken in gcc with c++11 set. #2039

Closed

yeswalrus commented Dec 1, 2020

View reviewed changes

vitaut requested changes Dec 3, 2020

View reviewed changes

yeswalrus mentioned this pull request Dec 4, 2020

Fix MSVC workaround with FMT_ENFORCE_COMPILE_STRING #2049

Closed

yeswalrus force-pushed the test_enforce_compile_string branch from b7ab26d to 270f664 Compare December 4, 2020 04:21

vitaut reviewed Dec 5, 2020

View reviewed changes

yeswalrus requested a review from vitaut December 9, 2020 19:28

vitaut requested changes Dec 12, 2020

View reviewed changes

yeswalrus force-pushed the test_enforce_compile_string branch from e4459fa to 82c47d3 Compare December 13, 2020 01:52

yeswalrus requested a review from vitaut December 13, 2020 02:02

vitaut requested changes Dec 19, 2020

View reviewed changes

Walter Gray added 8 commits December 20, 2020 16:07

Add test

e05c8fc

fix wide string formatting

f76c64a

fix formatted_size

b75bd49

fix chrono

2f9484c

clang-format, test fixes

3b6d21b

fix ranges

aa65e99

fix gcc test

54d6d5b

workaround MSVC bug

e805e8e

Walter Gray added 16 commits December 20, 2020 16:07

fix gcc pedantic error

900adeb

fix gcc errors

7f1c041

fix MSVC version check #if statements

d343e4b

trim down compiletime test

c4c8351

fixup chrono.h with better notes about MSVC workaround

60bceae

restore missing include

b61537a

cleanup doc string

d539f76

fixup test

dafbc05

fix unused arg issue in test

9fafb14

fix gcc issue

2489501

fixup chrono MSVC workaround

b8ebadd

Remove formatted_size overlaod accepting compile-time strings

098de0d

address comments

8e575bd

restore ifdef guarding GCC bug

f2a5a0f

address easy comments

c85d5e3

remove vformat from chrono.h

b9063f9

yeswalrus force-pushed the test_enforce_compile_string branch from 82c47d3 to b9063f9 Compare December 21, 2020 01:12

yeswalrus requested a review from vitaut December 21, 2020 05:16

attempt stripping void casts

98a8830

vitaut requested changes Dec 21, 2020

View reviewed changes

Refactor ranges, fix chrono, apply clang format to include/fmt/*.h

b0c65df

yeswalrus requested a review from vitaut December 21, 2020 19:18

fixup constexpr issues

6f393f2

vitaut requested changes Dec 23, 2020

View reviewed changes

Walter Gray added 3 commits December 23, 2020 19:05

revert locale format fix

06a576c

address feedback

99ed2a8

move array of literals format test

5083486

vitaut merged commit 4fa4c92 into fmtlib:master Dec 24, 2020

		Enforcing Compile-time Format Checks
		------------------------------------

		@@ -106,6 +106,10 @@ add_fmt_test(printf-test)
		add_fmt_test(ranges-test)
		add_fmt_test(scan-test)

		#if !FMT_GCC_VERSION // Currently will not compile: See
		// https://github.com/fmtlib/fmt/issues/2039

		basic_memory_buffer<Char> buffer;
		auto bufOut = std::back_inserter(buffer);

		OutputIt add_range_formatting_spaces(OutputIt out,
		const Formatting& formatting) {

Add tests for FMT_ENFORCE_COMPILE_STRING, fix several errors #2038

Add tests for FMT_ENFORCE_COMPILE_STRING, fix several errors #2038

Conversation

yeswalrus commented Nov 30, 2020 • edited

yeswalrus Dec 1, 2020 • edited

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

Choose a reason for hiding this comment

yeswalrus commented Dec 3, 2020

vitaut commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeswalrus commented Dec 9, 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

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

yeswalrus commented Dec 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeswalrus Dec 20, 2020 • edited

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

yeswalrus Dec 21, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeswalrus commented Dec 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeswalrus Dec 24, 2020 • edited

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 24, 2020

yeswalrus commented Nov 30, 2020 •

edited

yeswalrus Dec 1, 2020 •

edited

yeswalrus commented Dec 13, 2020 •

edited

yeswalrus Dec 20, 2020 •

edited

yeswalrus Dec 21, 2020 •

edited

yeswalrus Dec 24, 2020 •

edited