Skip to content
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

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

Closed
yeswalrus opened this issue Nov 10, 2020 · 9 comments
Closed

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

yeswalrus opened this issue Nov 10, 2020 · 9 comments

Comments

@yeswalrus
Copy link
Contributor

Several lines in format-inl.h do not use FMT_STRING, which causes static assertion failures with FMT_ENFORCE_COMPILE_STRING.

In file included from external/fmt/src/format.cc:8:
In file included from external/fmt/include/fmt/format-inl.h:28:
In file included from external/fmt/include/fmt/format.h:44:
external/fmt/include/fmt/core.h:522:3: error: static_assert failed due to requirement 'is_compile_string<char [7]>::value' "FMT_ENFORCE_COMPILE_STRING requires all format strings to use FMT_STRING."
  static_assert(is_compile_string<S>::value,
  ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~
external/fmt/include/fmt/core.h:1625:11: note: in instantiation of function template specialization 'fmt::v7::detail::check_format_string<fmt::v7::basic_string_view<char> &, char *&, char [7], 0>' requested here
  detail::check_format_string<Args...>(format_str);
          ^
external/fmt/include/fmt/core.h:2004:28: note: in instantiation of function template specialization 'fmt::v7::make_args_checked<fmt::v7::basic_string_view<char> &, char *&, char [7], char>' requested here
  const auto& vargs = fmt::make_args_checked<Args...>(format_str, args...);
                           ^
external/fmt/include/fmt/format-inl.h:2723:9: note: in instantiation of function template specialization 'fmt::v7::format_to<fmt::v7::detail::buffer_appender<char>, char [7], fmt::v7::basic_string_view<char> &, char *&, true>' requested here
        format_to(detail::buffer_appender<char>(out), "{}: {}", message,
@vitaut
Copy link
Contributor

vitaut commented Nov 10, 2020

Could you submit a PR to fix those?

@yeswalrus
Copy link
Contributor Author

I'm working on it and have a patch for just those files, but I've discovered a number of other places with issues in chrono.h that are much trickier. Do you know why the format strings there are given in the form of constant char arrays? ie const char format[] = {'{','}',0};

@vitaut
Copy link
Contributor

vitaut commented Nov 10, 2020

Do you know why the format strings there are given in the form of constant char arrays?

This is to handle the case when Char is not char.

@yeswalrus
Copy link
Contributor Author

Interesting. Is there any reason it would be bad to replace these with FMT_STRING("...") equivalents?

@yeswalrus
Copy link
Contributor Author

yeswalrus commented Nov 10, 2020

I've also discovered that using FMT_STRING() with fmt::format in color-test.cc is giving me lambda-expression in unevaluated context errors:

                from /home/walter.gray/development/fmt/test/color-test.cc:14:
/home/walter.gray/development/fmt/test/color-test.cc: In member function ‘virtual void ColorsTest_Format_Test::TestBody()’:
/home/walter.gray/development/fmt/include/fmt/format.h:3208:3: error: lambda-expression in unevaluated context
   [] {                                                                     \
   ^
/home/walter.gray/development/fmt/include/fmt/format.h:3231:23: note: in expansion of macro ‘FMT_STRING_IMPL’
 #define FMT_STRING(s) FMT_STRING_IMPL(s, fmt::compile_string)
                       ^~~~~~~~~~~~~~~
/home/walter.gray/development/fmt/test/color-test.cc:57:52: note: in expansion of macro ‘FMT_STRING’
   EXPECT_EQ(fmt::format(fg(fmt::rgb(255, 20, 30)), FMT_STRING("rgb(255,20,30)")),
                                                    ^~~~~~~~~~
/home/walter.gray/development/fmt/test/color-test.cc:57:3: error: template argument 1 is invalid
   EXPECT_EQ(fmt::format(fg(fmt::rgb(255, 20, 30)), FMT_STRING("rgb(255,20,30)")),

@vitaut
Copy link
Contributor

vitaut commented Nov 10, 2020

Is there any reason it would be bad to replace these with FMT_STRING("...") equivalents?

I don't think it will work.

I've also discovered that using FMT_STRING() with fmt::format in color-test.cc

Please don't change tests, only public headers.

@yeswalrus
Copy link
Contributor Author

yeswalrus commented Nov 11, 2020

#2006
Did my best. I'm not totally sure what to do in chrono.h, but the version I submitted works for me. It seems like it might be useful to make the tests use a TEST_FMT_STRING that could be toggled on/off to more thoroughly test which features work with FMT_STRING? Trying to do that is how I caught the additional issues in color & chrono.

@yeswalrus
Copy link
Contributor Author

Update: Got it working by manually calling vformat_to to avoid the check enabled by FMT_ENFORCE_COMPILE_STRING.

vitaut pushed a commit that referenced this issue Nov 15, 2020
@vitaut vitaut closed this as completed Nov 15, 2020
@vitaut
Copy link
Contributor

vitaut commented Nov 15, 2020

It seems like it might be useful to make the tests use a TEST_FMT_STRING that could be toggled on/off to more thoroughly test which features work with FMT_STRING?

I don't think it's worth the trouble but a PR to add one test that checks that FMT_ENFORCE_COMPILE_STRING is working would be welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants