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

several -Wconversion and/or -Wsign-conversion warnings on MINGW when formatting std::chrono::system_clock::now() #4268

Open
oschonrock opened this issue Dec 18, 2024 · 5 comments

Comments

@oschonrock
Copy link

oschonrock commented Dec 18, 2024

Note: on release fmtlib 11.02 this was only occurring on the MINGW64 msys2 environment , not on MINGW32 nor on UCRT64 nor on CLANG64, and the warning was quite different. It was a -Wconversion (now a -Wsign-conversion):

fmtlib 11.02 warning on MINGW64

D:/a/hibp/hibp/include/dnl/shared.hpp:70:31:   required from here
   70 |       std::cerr << fmt::format("{:%Y-%m-%d %H:%M:%S} thread: {:>9}: {}\n", timestamp,
      |                    ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   71 |                                thrnames[std::this_thread::get_id()], msg);
      |                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
D:/a/hibp/hibp/ext/fmt/include/fmt/chrono.h:1371:22: warning: conversion from 'time_t' {aka 'long long int'} to 'long int' may change value [-Wconversion]
 1371 |     long offset = gt - lt;
      |                   ~~~^~~~

As part of reporting this issues, when I built with latest master, and there are now several warnings from the same c++ code on MINGW64, UCRT64, MINGW32 (but not on CLANG64). So the obviously different implementation on latest master arguably "contains a regression" with respect to conversion/sign warnings on several msys2 environments. I have not tested fmtlib@master/HEAD on "normal" gcc environment like ubuntu, but given the nature of the warnings below, I suspect they may occur there too.

Being thrown on my msys2 matrix CI tests, and stops me running with -Werror.

c++ code to reproduce:

#include <iostream>
#include <chrono>
#include "fmt/format.h"
#include "fmt/chrono.h"

int main() {
  auto timestamp = std::chrono::system_clock::now();
  std::cerr << fmt::format("{:%Y-%m-%d %H:%M:%S}\n", timestamp);
}

fmtlib fresh checkout from master branch:

git clone https://github.com/fmtlib/fmt.git

cmake config:

cmake_minimum_required(VERSION 3.16)
project(fmtlib_test)

set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)

list(APPEND PROJECT_COMPILE_OPTIONS -Wall -Wextra -Wpedantic -Wshadow -Wextra-semi
  -Wmissing-noreturn -Wconversion -Wsign-conversion -Wno-ignored-attributes)

add_subdirectory(ext/fmt EXCLUDE_FROM_ALL)

add_executable(fmtlib_test fmtlib_test.cpp)
target_compile_features(fmtlib_test PRIVATE cxx_std_20)
target_compile_options(fmtlib_test PRIVATE ${PROJECT_COMPILE_OPTIONS})
target_link_libraries(fmtlib_test PRIVATE fmt::fmt)

compile command on updated MINGW64, UCRT64, MINGW32 (but not on CLANG64), which triggers warning

 C:\msys64\mingw64\bin\c++.exe  -IC:/msys64/home/oliver/fmtlib_test/ext/fmt/include -std=c++20 -Wall -Wextra -Wpedantic -Wshadow -Wextra-semi -Wmissing-noreturn -Wconversion -Wsign-conversion -Wno-ignored-attributes -MD -MT CM
akeFiles/fmtlib_test.dir/fmtlib_test.cpp.obj -MF CMakeFiles\fmtlib_test.dir\fmtlib_test.cpp.obj.d -o CMakeFiles/fmtlib_test.dir/fmtlib_test.cpp.obj -c C:/msys64/home/oliver/fmtlib_test/fmtlib_test.cpp

warnings:

In file included from C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:41,
                 from C:/msys64/home/oliver/fmtlib_test/fmtlib_test.cpp:3:
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:1641:21: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 1641 |   type types_[max_of(1, NUM_ARGS)];
      |               ~~~~~~^~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:1642:42: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 1642 |   named_arg_info<Char> named_args_[max_of(1, NUM_NAMED_ARGS)];
      |                                    ~~~~~~^~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:1646:33: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 1646 |   parse_func parse_funcs_[max_of(1, NUM_ARGS)];
      |                           ~~~~~~^~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:2291:35: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 2291 |   arg_t<Context, NUM_ARGS> args[1 + NUM_ARGS];
      |                                 ~~^~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:2292:72: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 2292 |   named_arg_info<typename Context::char_type> named_args[NUM_NAMED_ARGS];
      |                                                                        ^
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:2325:52: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 2325 |                     arg_t<Context, NUM_ARGS>[max_of(1, NUM_ARGS)],
      |                                              ~~~~~~^~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h: In function 'constexpr OutputIt fmt::v11::detail::write_int(OutputIt, write_int_arg<T>, const fmt::v11::format_specs&)':
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:2032:15: warning: conversion to 'long long unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 2032 |   char buffer[buffer_size];
      |               ^~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h: In instantiation of 'constexpr Char* fmt::v11::detail::do_format_base2e(int, Char*, UInt, int, bool) [with Char = char; UInt = unsigned int]':
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:1251:19:   required from 'constexpr Char* fmt::v11::detail::format_base2e(int, Char*, UInt, int, bool) [with Char = char; UInt = unsigned int]'
 1251 |   do_format_base2e(base_bits, out, value, num_digits, upper);
      |   ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:1740:16:   required from 'OutputIt fmt::v11::detail::write_codepoint(OutputIt, char, uint32_t) [with long long unsigned int width = 2; Char = char; OutputIt = fmt::v11:
:basic_appender<char>; uint32_t = unsigned int]'
 1740 |   format_base2e(4, buf, cp, width);
      |   ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:1765:59:   required from 'OutputIt fmt::v11::detail::write_escaped_cp(OutputIt, const find_escape_result<Char>&) [with OutputIt = fmt::v11::basic_appender<char>; Char =
 char]'
 1765 |     if (escape.cp < 0x100) return write_codepoint<2, Char>(out, 'x', escape.cp);
      |                                   ~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:1791:43:   required from 'OutputIt fmt::v11::detail::write_escaped_string(OutputIt, fmt::v11::basic_string_view<Char>) [with Char = char; OutputIt = fmt::v11::basic_app
ender<char>]'
 1791 |     out = write_escaped_cp<OutputIt, Char>(out, escape);
      |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:2134:25:   [ skipping 2 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/chrono.h:2299:47:   required from 'decltype (ctx.out()) fmt::v11::formatter<std::chrono::time_point<std::chrono::_V2::system_clock, _Duration>, Char>::format(fmt::v11::sys_time<
Duration>, FormatContext&) const [with FormatContext = fmt::v11::context; Char = char; Duration = std::chrono::duration<long long int, std::ratio<1, 1000000000> >; decltype (ctx.out()) = fmt::v11::basic_appender<char>; fmt::v11::sy
s_time<Duration> = std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long long int, std::ratio<1, 1000000000> > >]'
 2299 |     return formatter<std::tm, Char>::do_format(tm, ctx, &subsecs);
      |            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:2224:29:   required from 'static void fmt::v11::detail::value<Context>::format_custom(void*, fmt::v11::parse_context<typename Context::char_type>&, Context&) [with T = st
d::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long long int, std::ratio<1, 1000000000> > >; Formatter = fmt::v11::formatter<std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration
<long long int, std::ratio<1, 1000000000> > >, char, void>; Context = fmt::v11::context; typename Context::char_type = char]'
 2224 |     ctx.advance_to(cf.format(*static_cast<qualified_type*>(arg), ctx));
      |                    ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:2204:19:   required from 'constexpr fmt::v11::detail::value<Context>::value(T&, fmt::v11::detail::custom_tag) [with T = std::chrono::time_point<std::chrono::_V2::system_c
lock, std::chrono::duration<long long int, std::ratio<1, 1000000000> > >; typename std::enable_if<has_formatter<T, typename Context::char_type>(), int>::type <anonymous> = 0; Context = fmt::v11::context]'
 2204 |     custom.format = format_custom<value_type, formatter<value_type, char_type>>;
      |     ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/base.h:2184:65:   required from 'constexpr fmt::v11::detail::value<Context>::value(T&) [with T = std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<lo
ng long int, std::ratio<1, 1000000000> > >; typename std::enable_if<(fmt::v11::detail::use_formatter<T>::value || (!1)), int>::type <anonymous> = 0; Context = fmt::v11::context]'
 2184 |   FMT_CONSTEXPR20 FMT_INLINE value(T& x) : value(x, custom_tag()) {}
      |                                                                 ^
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:4190:17:   required from 'std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::du
ration<long long int, std::ratio<1, 1000000000> > >&}; std::string = std::__cxx11::basic_string<char>; format_string<T ...> = fstring<std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long long int, std:
:ratio<1, 1000000000> > >&>]'
 4190 |   return vformat(fmt.str, vargs<T...>{{args...}});
      |          ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/fmtlib_test.cpp:8:27:   required from here
    8 |   std::cerr << fmt::format("{:%Y-%m-%d %H:%M:%S}\n", timestamp);
      |                ~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:/msys64/home/oliver/fmtlib_test/ext/fmt/include/fmt/format.h:1240:70: warning: conversion to 'unsigned int' from 'int' may change the sign of the result [-Wsign-conversion]
 1240 |     unsigned digit = static_cast<unsigned>(value & ((1 << base_bits) - 1));
      |                                                    ~~~~~~~~~~~~~~~~~~^~~~

@oschonrock oschonrock changed the title -Wconversion warning on MINGW64 when formatting std::chrono::system_clock::now() several -Wconversion warnings on MINGW64 when formatting std::chrono::system_clock::now() Dec 18, 2024
@oschonrock oschonrock changed the title several -Wconversion warnings on MINGW64 when formatting std::chrono::system_clock::now() several -Wconversion warnings on MINGW when formatting std::chrono::system_clock::now() Dec 18, 2024
@oschonrock oschonrock changed the title several -Wconversion warnings on MINGW when formatting std::chrono::system_clock::now() several -Wconversion and/or -Wsign-conversion warnings on MINGW when formatting std::chrono::system_clock::now() Dec 18, 2024
@mwinterb
Copy link
Contributor

The first warning (long offset = ...) was fixed in fe79932.

@vitaut
Copy link
Contributor

vitaut commented Dec 20, 2024

Could you provide a godbolt repro?

@oschonrock
Copy link
Author

oschonrock commented Dec 20, 2024

I don't believe godbolt supports fmtlib with MINGW?

https://godbolt.org/z/sKcefWbao

@oschonrock
Copy link
Author

oschonrock commented Dec 20, 2024

As I mentioned above, I thought most of these fmt trunk warning should be reproducible on any platform.

I have now tried and succeeded to reproduce a very similar set of warnings under ubuntu 24.04 gcc13.3 (gcc 14.2 also shows them). See warnings below with gcc command line.

But slightly annoyingly, godbolt does not show these. https://godbolt.org/z/qP9qGrYh9

I have selected fmtlib/trunk on that godbolt link, but how often does godbolt actually git pull, because the lines in the warnings below are actually relatively new, Most of them seem to be from this commit:

3faf6f1

$ g++ -Ifmt/include -std=c++20 -Wall -Wextra -Wconversion -Wsign-conversion   -o fmt  fmt.cpp

In file included from fmt/include/fmt/format.h:41,
                 from fmt.cpp:3:
fmt/include/fmt/base.h:1641:21: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 1641 |   type types_[max_of(1, NUM_ARGS)];
      |               ~~~~~~^~~~~~~~~~~~~
fmt/include/fmt/base.h:1642:42: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 1642 |   named_arg_info<Char> named_args_[max_of(1, NUM_NAMED_ARGS)];
      |                                    ~~~~~~^~~~~~~~~~~~~~~~~~~
fmt/include/fmt/base.h:1646:33: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 1646 |   parse_func parse_funcs_[max_of(1, NUM_ARGS)];
      |                           ~~~~~~^~~~~~~~~~~~~
fmt/include/fmt/base.h:2291:35: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 2291 |   arg_t<Context, NUM_ARGS> args[1 + NUM_ARGS];
      |                                 ~~^~~~~~~~~~
fmt/include/fmt/base.h:2292:72: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 2292 |   named_arg_info<typename Context::char_type> named_args[NUM_NAMED_ARGS];
      |                                                                        ^
fmt/include/fmt/base.h:2325:52: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 2325 |                     arg_t<Context, NUM_ARGS>[max_of(1, NUM_ARGS)],
      |                                              ~~~~~~^~~~~~~~~~~~~
fmt/include/fmt/format.h: In function ‘constexpr OutputIt fmt::v11::detail::write_int(OutputIt, write_int_arg<T>, const fmt::v11::format_specs&)’:
fmt/include/fmt/format.h:2032:15: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 2032 |   char buffer[buffer_size];
      |               ^~~~~~~~~~~
fmt/include/fmt/format.h: In instantiation of ‘constexpr Char* fmt::v11::detail::do_format_base2e(int, Char*, UInt, int, bool) [with Char = char; UInt = unsigned int]’:
fmt/include/fmt/format.h:1251:19:   required from ‘constexpr Char* fmt::v11::detail::format_base2e(int, Char*, UInt, int, bool) [with Char = char; UInt = unsigned int]’
fmt/include/fmt/format.h:1740:16:   required from ‘OutputIt fmt::v11::detail::write_codepoint(OutputIt, char, uint32_t) [with long unsigned int width = 2; Char = char; OutputIt = fmt::v11::basic_appender<char>; uint32_t = unsigned int]’
fmt/include/fmt/format.h:1765:59:   required from ‘OutputIt fmt::v11::detail::write_escaped_cp(OutputIt, const find_escape_result<Char>&) [with OutputIt = fmt::v11::basic_appender<char>; Char = char]’
fmt/include/fmt/format.h:1791:43:   required from ‘OutputIt fmt::v11::detail::write_escaped_string(OutputIt, fmt::v11::basic_string_view<Char>) [with Char = char; OutputIt = fmt::v11::basic_appender<char>]’
fmt/include/fmt/format.h:2134:25:   [ skipping 2 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
fmt/include/fmt/chrono.h:2299:47:   required from ‘decltype (ctx.out()) fmt::v11::formatter<std::chrono::time_point<std::chrono::_V2::system_clock, _Duration>, Char>::format(fmt::v11::sys_time<Duration>, FormatContext&) const [with FormatContext = fmt::v11::context; Char = char; Duration = std::chrono::duration<long int, std::ratio<1, 1000000000> >; decltype (ctx.out()) = fmt::v11::basic_appender<char>; fmt::v11::sys_time<Duration> = std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >]’
fmt/include/fmt/base.h:2224:29:   required from ‘static void fmt::v11::detail::value<Context>::format_custom(void*, fmt::v11::parse_context<typename Context::char_type>&, Context&) [with T = std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >; Formatter = fmt::v11::formatter<std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >, char, void>; Context = fmt::v11::context; typename Context::char_type = char]’
fmt/include/fmt/base.h:2204:19:   required from ‘constexpr fmt::v11::detail::value<Context>::value(T&, fmt::v11::detail::custom_tag) [with T = std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >; typename std::enable_if<has_formatter<T, typename Context::char_type>(), int>::type <anonymous> = 0; Context = fmt::v11::context]’
fmt/include/fmt/base.h:2184:65:   required from ‘constexpr fmt::v11::detail::value<Context>::value(T&) [with T = std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >; typename std::enable_if<(fmt::v11::detail::use_formatter<T>::value || (!1)), int>::type <anonymous> = 0; Context = fmt::v11::context]’
fmt/include/fmt/format.h:4190:17:   required from ‘std::string fmt::v11::format(format_string<T ...>, T&& ...) [with T = {std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >&}; std::string = std::__cxx11::basic_string<char>; format_string<T ...> = fstring<std::chrono::time_point<std::chrono::_V2::system_clock, std::chrono::duration<long int, std::ratio<1, 1000000000> > >&>]’
fmt.cpp:8:27:   required from here
fmt/include/fmt/format.h:1240:70: warning: conversion to ‘unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
 1240 |     unsigned digit = static_cast<unsigned>(value & ((1 << base_bits) - 1));
      |                                                    ~~~~~~~~~~~~~~~~~~^~~~

@vitaut
Copy link
Contributor

vitaut commented Dec 20, 2024

I have now tried and succeeded to reproduce a very similar set of warnings under ubuntu 24.04 gcc13.3

OK, these are sufficient repro details, we don't need godbolt.

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

3 participants