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

Fix compilation errors due to make_format_args in gcc 14.1.1 with c++20 #4042

Merged
merged 8 commits into from
Jul 3, 2024

Conversation

toge
Copy link
Contributor

@toge toge commented Jul 1, 2024

In gcc 14.1.1 with c++20, there are compilation errors following code:

// g++ --std=c++20 test.cpp -lfmt
#include <fmt/printf.h>
#include <fmt/ostream.h>

int main() {
    const std::string thing("World");
    fmt::printf("PRINTF: Hello, %s!\n", thing);
    return 0;
}

The error messages are like this:

In file included from test_package.cpp:10:
include/fmt/printf.h: In instantiation of 'int fmt::v10::printf(string_view, const T& ...) [with T = {std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >}; string_view = basic_string_view<char>]':
test_package.cpp:16:16:   required from here
   16 |     fmt::printf("PRINTF: Hello, %s!\n", thing);
      |     ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/fmt/printf.h:645:48: error: no matching function for call to 'make_printf_args(const std::__cxx11::basic_string<char>&)'
  645 |   return vfprintf(stdout, fmt, make_printf_args(args...));
      |                                ~~~~~~~~~~~~~~~~^~~~~~~~~
include/fmt/printf.h:571:13: note: candidate: 'template<class Char, class ... T> decltype (make_format_args<fmt::v10::basic_printf_context<Char> >(fmt::v10::make_printf_args::args ...)) fmt::v10::make_printf_args(T& ...)'
  571 | inline auto make_printf_args(T&... args)
      |             ^~~~~~~~~~~~~~~~
include/fmt/printf.h:571:13: note:   template argument deduction/substitution failed:
include/fmt/printf.h: In substitution of 'template<class Char, class ... T> decltype (make_format_args<fmt::v10::basic_printf_context<Char> >(fmt::v10::make_printf_args::args ...)) fmt::v10::make_printf_args(T& ...) [with Char = char; T = {const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >}]':
include/fmt/printf.h:645:48:   required from 'int fmt::v10::printf(string_view, const T& ...) [with T = {std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >}; string_view = basic_string_view<char>]'
  645 |   return vfprintf(stdout, fmt, make_printf_args(args...));
      |                                ~~~~~~~~~~~~~~~~^~~~~~~~~
test_package.cpp:16:16:   required from here
   16 |     fmt::printf("PRINTF: Hello, %s!\n", thing);
      |     ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/fmt/printf.h:572:61: error: call of overloaded 'make_format_args<fmt::v10::basic_printf_context<char> >(const std::__cxx11::basic_string<char>&)' is ambiguous
  572 |     -> decltype(make_format_args<basic_printf_context<Char>>(args...)) {
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
In file included from include/fmt/format.h:41,
                 from include/fmt/printf.h:16:
include/fmt/base.h:2000:34: note: candidate: 'constexpr fmt::v10::detail::format_arg_store<Context, NUM_ARGS, 0, DESC> fmt::v10::make_format_args(T& ...) [with Context = basic_printf_context<char>; T = {const std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >}; long unsigned int NUM_ARGS = 1; long unsigned int NUM_NAMED_ARGS = 0; long long unsigned int DESC = 13; typename std::enable_if<(NUM_NAMED_ARGS == 0), int>::type <anonymous> = 0]'
 2000 | constexpr FMT_ALWAYS_INLINE auto make_format_args(T&... args)
      |                                  ^~~~~~~~~~~~~~~~
In file included from /usr/include/c++/14/bits/chrono_io.h:39,
                 from /usr/include/c++/14/chrono:3360,
                 from include/fmt/chrono.h:13,
                 from include/fmt/ostream.h:23,
                 from test_package.cpp:11:
/usr/include/c++/14/format:3799:5: note: candidate: 'auto std::make_format_args(_Args& ...) [with _Context = fmt::v10::basic_printf_context<char>; _Args = {const __cxx11::basic_string<char, char_traits<char>, allocator<char> >}]'
 3799 |     make_format_args(_Args&... __fmt_args) noexcept
      |     ^~~~~~~~~~~~~~~~

It seems to be caused by recursive make_printf_args calls without namespace.
This PR tries to solve it.

I think this is an adhoc approach, so if there is a more elegant way,
I will gladly close this PR.

@vitaut
Copy link
Contributor

vitaut commented Jul 1, 2024

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! The fix looks correct but could you add a test case to https://github.com/fmtlib/fmt/blob/master/test/printf-test.cc to prevent regression?

@toge
Copy link
Contributor Author

toge commented Jul 2, 2024

@vitaut
I update printf-test.cc and CMakeLists.txt for checking this situation.
Please review it again.

Copy link
Contributor

@vitaut vitaut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some .gradle files were added accidentally.

Comment on lines 47 to 51
# use C++20 if C++20 is available for https://github.com/fmtlib/fmt/pull/4042
get_property(cxx_features GLOBAL PROPERTY CMAKE_CXX_KNOWN_FEATURES)
if(cxx_std_20 IN_LIST cxx_features)
target_compile_features(${name} PRIVATE cxx_std_20)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This standard should be controlled externally, not here. Let's revert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitaut
Sorry I try to revert both cxx_std and .gradle files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vitaut
Now I finished.
Cloud you review it again?

@@ -6,6 +6,10 @@
// For the license information refer to format.h.

#include "fmt/printf.h"
// include <format> if possible for https://github.com/fmtlib/fmt/pull/4042
#if defined(__has_include) && __has_include(<format>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#if FMT_HAS_INCLUDE(<format>) && FMT_CPLUSPLUS > 201703L

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phprus
Thanks a lot!
Fixed.

@vitaut vitaut merged commit 9d946a2 into fmtlib:master Jul 3, 2024
43 checks passed
@vitaut
Copy link
Contributor

vitaut commented Jul 3, 2024

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants