FMT_COMPILE tests are temporary switched on even if constexpr_if is n…

[YarikTH]

I mistakenly tried to compile code with FMT_COMPILE without setting c++ standard to c++17 or upper. And as result, I saw a sexy error output that has to be meditated on to figure out what's happened. And it looks like a library bug at the first glance despite it isn't.

<path>/fmt-master/include/fmt/compile.h: In instantiation of ‘constexpr OutputIt fmt::v7::format_to(OutputIt, const S&, const Args& ...) [with OutputIt = char*; S = test_functions::test_bundled(char*)::<lambda()>::FMT_COMPILE_STRING; Args = {int, float, int, char [7], int}; typename std::enable_if<fmt::v7::detail::is_compiled_string<S>::value, int>::type <anonymous> = 0]’:
<path>/test_bundled.cpp:6:58:   required from here
<path>/fmt-master/include/fmt/compile.h:695:53: error: no matching function for call to ‘compile<int, float, int, char [7], int>(test_functions::test_bundled(char*)::<lambda()>::FMT_COMPILE_STRING)’
  695 |   constexpr auto compiled = detail::compile<Args...>(S());
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
<path>/fmt-master/include/fmt/compile.h:611:16: note: candidate: ‘template<class ... Args, class S, typename std::enable_if<fmt::v7::is_compile_string<S>::value, int>::type <anonymous> > constexpr fmt::v7::detail::compiled_format<S, Args ...> fmt::v7::detail::compile(S)’
  611 | constexpr auto compile(S format_str) -> detail::compiled_format<S, Args...> {
      |                ^~~~~~~
<path>/fmt-master/include/fmt/compile.h:611:16: note:   template argument deduction/substitution failed:
In file included from <path>/fmt-master/include/fmt/format.h:45,
                 from <path>/fmt-master/include/fmt/compile.h:14,
                 from <path>/test_bundled.cpp:1:
<path>/fmt-master/include/fmt/core.h:281:64: error: no type named ‘type’ in ‘struct std::enable_if<false, int>’
  281 | #  define FMT_ENABLE_IF(...) enable_if_t<(__VA_ARGS__), int> = 0
      |                                                                ^
<path>/fmt-master/include/fmt/compile.h:610:11: note: in expansion of macro ‘FMT_ENABLE_IF’
  610 |           FMT_ENABLE_IF(is_compile_string<S>::value)>
      |           ^~~~~~~~~~~~~
In file included from <path>/test_bundled.cpp:1:
<path>/fmt-master/include/fmt/compile.h:618:6: note: candidate: ‘template<class ... Args, class Char, long unsigned int N> fmt::v7::detail::compiled_format<const Char*, Args ...> fmt::v7::detail::compile(const Char (&)[N])’
  618 | auto compile(const Char (&format_str)[N])
      |      ^~~~~~~
<path>/fmt-master/include/fmt/compile.h:618:6: note:   template argument deduction/substitution failed:
<path>/fmt-master/include/fmt/compile.h:695:53: note:   mismatched types ‘const Char [N]’ and ‘test_functions::test_bundled(char*)::<lambda()>::FMT_COMPILE_STRING’
  695 |   constexpr auto compiled = detail::compile<Args...>(S());
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~

The compiler is g++9.3, the standard is c++11 (output of c++14 is the same)

The documentation says that FMT_COMPILE "requires C++17 constexpr if compiler support", but who reads the documentation? And in my case, I didn't use FMT_COMPILE but received someone else's code instead, so I had no idea what of {fmt} features were used and which are their requirements.

/**
  \rst
  Converts a string literal *s* into a format string that will be parsed at
  compile time and converted into efficient formatting code. Requires C++17
  ``constexpr if`` compiler support.

  **Example**::

    // Converts 42 into std::string using the most efficient method and no
    // runtime format string processing.
    std::string s = fmt::format(FMT_COMPILE("{}"), 42);
  \endrst
 */
#define FMT_COMPILE(s)

I propose to add a better compile-time error explanation. Like the following:

<path>/fmt_compile_before.hpp: In instantiation of ‘const char* fmt::v7::detail::report_fmt_compile_error() [with T = void*]’:
<path>/test_before.cpp:6:26:   required from here
<path>/fmt_compile_before.hpp:37:74: error: static assertion failed: FMT_COMPILE requires C++17 `constexpr if` compiler support
   37 | template<class T> const char* report_fmt_compile_error() { static_assert(!std::is_same<T,T>::value, "FMT_COMPILE requires C++17 `constexpr if` compiler support"); return ""; }
      |                                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~
      

To reproduce it a trivial code is sufficient:

#include "fmt/compile.h"

int main() {
    fmt::format(FMT_COMPILE("{}"), 42);
}

https://godbolt.org/z/T8M88b

I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.

[YarikTH]

"include/fmt/compile.h" is ready for review. Other changes will be reverted when all issues with target code will be fixed.

[alexezeder]

My thoughts on this PR, maybe they will be useful

[alexezeder]

Could it be done without introducing this additional type?

[YarikTH]

Do you mean false_type? It's optional, but error messages of GCC and clang will be a bit uglier, because in case of static_assert, its condition is highlighted. Like following:

static_assert(!std::is_same<T,T>::value, "FMT_COMPILE requires C++17 `constexpr if` compiler support"); return ""; }
 |            ^~~~~~~~~~~~~~~~~~~~~~~~~

If code in the condition is not obvious then it attends too many unwanted attention. I fear decisions with storing value locally in the named constexpr bool or constant alias don't work on the all supported platforms. Maybe they do. I'm not sure.

  template<class T> FMT_CONSTEXPR const char* report_compile_error() {
    using false_v = !std::is_same<T,T>::value;
    // constexpr bool false_v = !std::is_same<T,T>::value;
    static_assert(
        false_v, "FMT_COMPILE requires C++17 `constexpr if` compiler support"
    );
    return "";
  }

[YarikTH]

Btw, I added additional namespace fmt_compile_error just not to spoil namespace with the specific code.

[alexezeder]

The error is something like:

error: static assertion failed: FMT_COMPILE requires C++17 `constexpr if` compiler support

that can have the expression of course, but the first line has static_assert string only on most compilers.

[alexezeder]

Seems like broken code-style for me

[YarikTH]

If you mean that both scopes are on separate lines, while arguments are indented and are on separate line? It was done by purpose. Because GCC and clang output full line with the failed static_assert's condition. So I think that it's better to place not so interesting parts of this code aside.

[alexezeder]

IMHO, code style should be either applied for entire code or disabled with special //clang format off directive.
Don't think that you as the end user would somehow analyze the code that fail static_assert since you already have verbose error with static_assert string.

[alexezeder]

Could it use the same macro as it used to use before to avoid another macro expansion? Probably not because of docs, but maybe there is a solution for that...

[YarikTH]

We'll have an issue with documentation then. I mean if I use

#ifdef __cpp_if_constexpr
#  define FMT_COMPILE(s) FMT_STRING_IMPL(s, fmt::detail::compiled_string)
#else
#  define FMT_COMPILE(s) <error verion>
#endif

But if there is another macro like FMT_CONDITIONAL( __cpp_if_constexpr, FMT_STRING_IMPL(s, fmt::detail::compiled_string), <error verion> )
Then it might work. But FMT_CONDITIONAL will add inderection level too.

Also, the following version might work but is kind of ugly

#define FMT_COMPILE(s) FMT_STRING_IMPL(s, fmt::detail::compiled_string)

#ifdef __cpp_if_constexpr
#undef FMT_COMPILE
#  define FMT_COMPILE(s) <error verion>
#endif

I don't see a problem with extra macro expansion anyway.

[vitaut]

Thanks for the PR but the plan is to make FMT_COMPILE fallback to runtime formatting when the compiler doesn't support necessary features. This is similar to FMT_STRING's behavior.