Clean-up sign-conversion warnings

[0x8000-0000]

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

[0x8000-0000]

This first step reduces the number of -Wsign-conversion warnings from 317 to 58 with GCC9.

[0x8000-0000]

I have tried to reproduce the test crash on Visual Studio 2019 building in Release Mode for 32 bits (I don't have 2017)

> "C:\Program Files (x86)\Microsoft Visual Studio\2019\Preview\Common7\Tools\VsDevCmd.bat" -arch=x86

> cmake ..\fmt -DFMT_PEDANTIC=ON -DCMAKE_BUILD_TYPE=Release -G"Visual Studio 16 2019"

> cmake --build . --config Release

> ctest -C Release

And I can't reproduce the crash. If somebody has access to that toolchain and could try debugging it, I would be very grateful. Otherwise I would have to do a binary search on all the changes in this commit :(

[0x8000-0000]

Made doubly-sure I have produced 32 bit binaries with:

"C:\Program Files (x86)\Microsoft Visual Studio\2019\Professional\Common7\IDE\CommonExtensions\Microsoft\CMake\CMake\bin\cmake" ..\fmt -DFMT_PEDANTIC=ON -DCMAKE_BUILD_TYPE=Release -A win32

$ file bin/Release/color-test.exe
bin/Release/color-test.exe: PE32 executable (console) Intel 80386, for MS Windows

Still, no crash when running the 32-bit binary on Windows 64, running on AMD laptop (FWIW)

[0x8000-0000]

This still crashes in UtilTest.WriteUIntPtr, even with just some changes in chrono.h ! I suspect the master branch just fails in the same way at this time.

@vitaut can you please try kicking a regression off master branch and see if it fails on Windows / 32bit Release with Visual Studio 2017?

[0x8000-0000]

Oh, it "fails" in the same way as #1438 , even though that one just modifies a text document.

[vitaut]

Yeah, WriteUIntPtr test failure is unrelated. I'm about to fix it (just waiting for see if CI is green).

[vitaut]

Merged the fix into master. Please rebase.

[0x8000-0000]

Down to 51 errors on GCC9, 18 on Clang9.

[0x8000-0000]

Down to 1 warning in fmt code, reported by both Gcc9 and Clang9.

fmt/test/format-test.cc:2026:10: error: implicit conversion changes signedness: 'unsigned __int128' to '::testing::internal::Function<void (__int128_t)>::Argument1' (aka '__int128') [-Werror,-Wsign-conversion]

Gcc9 also reports several warnings stemming from a macro in test/gtest/gtest.h:

1922 #define GTEST_COMPILE_ASSERT_(expr, msg) \
1923 typedef ::testing::internal::CompileAssert<(static_cast(expr))> \
1924 msg[static_cast(expr) ? 1 : -1] GTEST_ATTRIBUTE_UNUSED_

I think there are some cases where Gcc9 sees "msg[-1]" and reports the warning. It might be worth exploring whether upgrading gtest/gmock would clean that up.

[vitaut]

Thanks for the PR.

[vitaut]

This is unnecessary for the test purposes. Just static cast to the largest signed type.

[0x8000-0000]

The test is calling the mocked method with both signed and unsigned values.

I don't care as much about the tests being -Wsign-conversion clean, but...

I want my own code to be -Wsign-conversion clean, because we got bitten in the past by (unsigned)-1, and other bit fields confusion (somebody shoving 32 bits into a 16 bit register) and I want to use {fmt}. Which means {fmt} needs to be -Wsign-conversion clean, which means its test suite needs to be -Wsign-conversion clean, otherwise we can't enforce it via the automated regression suite.

[vitaut]

This is a simple mock which is only used here:

fmt/test/format-test.cc

Line 2052 in 62da1db

TEST(FormatTest, CustomArgFormatter) { custom_format("{}", 42); }

so casting is fine.

[0x8000-0000]

I changed as suggested and I got back the dreaded:

fmt/test/format-test.cc:2026:10: error: implicit conversion changes signedness: 'unsigned __int128' to '::testing::internal::Function<void (__int128_t)>::Argument1' (aka '__int128') [-Werror,-Wsign-conversion]

[0x8000-0000]

This is the long story, threaded through format.h and core.h:

/home/florin/libraries/fmt/test/format-test.cc: In instantiation of ‘typename std::enable_if<fmt::v6::internal::is_integral::value, std::back_insert_iterator<fmt::v6::internal::buffer > >::type mock_arg_formatter::operator()(T) [with T = __int128 unsigned; typename std::enable_if<fmt::v6::internal::is_integral::value, std::back_insert_iterator<fmt::v6::internal::buffer > >::type = std::back_insert_iterator<fmt::v6::internal::buffer >]’:

/home/florin/libraries/fmt/include/fmt/core.h:1024:15: required from ‘constexpr decltype (vis(0)) fmt::v6::visit_format_arg(Visitor&&, const fmt::v6::basic_format_arg&) [with Visitor = mock_arg_formatter; Context = fmt::v6::basic_format_context<std::back_insert_iterator<fmt::v6::internal::buffer >, char>; decltype (vis(0)) = std::back_insert_iterator<fmt::v6::internal::buffer >]’

/home/florin/libraries/fmt/include/fmt/format.h:3172:25: required from ‘void fmt::v6::format_handler<ArgFormatter, Char, Context>::on_replacement_field(const Char*) [with ArgFormatter = mock_arg_formatter; Char = char; Context = fmt::v6::basic_format_context<std::back_insert_iterator<fmt::v6::internal::buffer >, char>]’

/home/florin/libraries/fmt/include/fmt/format.h:2583:7: required from ‘constexpr void fmt::v6::internal::parse_format_string(fmt::v6::basic_string_view, Handler&&) [with bool IS_CONSTEXPR = false; Char = char; Handler = fmt::v6::format_handler<mock_arg_formatter, char, fmt::v6::basic_format_context<std::back_insert_iterator<fmt::v6::internal::buffer >, char> >&]’

/home/florin/libraries/fmt/include/fmt/format.h:3205:39: required from ‘typename Context::iterator fmt::v6::vformat_to(typename ArgFormatter::range, fmt::v6::basic_string_view, fmt::v6::basic_format_args, fmt::v6::internal::locale_ref) [with ArgFormatter = mock_arg_formatter; Char = char; Context = fmt::v6::basic_format_context<std::back_insert_iterator<fmt::v6::internal::buffer >, char>; typename Context::iterator = std::back_insert_iterator<fmt::v6::internal::buffer >; typename ArgFormatter::range = fmt::v6::buffer_range]’

/home/florin/libraries/fmt/test/format-test.cc:2043:63: required from here
/home/florin/libraries/fmt/test/format-test.cc:2026:10: error: conversion to ‘testing::internal::Function<void(__int128)>::Argument1’ {aka ‘__int128’} from ‘__int128 unsigned’ may change the sign of the result [-Werror=sign-conversion]
2026 | call(value);
| ^~~~~

[vitaut]

This tests an example from the standard and should remain unchanged.

[0x8000-0000]

Should width_arg_id become signed then?

[0x8000-0000]

width_arg_id is passed on the next line to check_arg_id which requires a size_t .

[0x8000-0000]

Once this is approved I can refactor the top commit and push the changes into the earlier commits. I didn't want to force-push during the review.

[vitaut]

I guess this can be closed now in favor of #1440.