Fix: libdnf5-cli: TransactionSummary counters data type #1701
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes "error: implicit conversion changes signedness: 'const int' to 'unsigned long' [-Werror,-Wsign-conversion]" The bug was caused by commit "I18N: Mark messages in "dnf install" output for a translation"
|
Sorry. I did not notice that ngettext() expects an unsigned long int as a counter. Shouldn't we add -Wsign-conversion to CXXFLAGS so that GCC catches it too? |
Good idea. I tried, but I must have done it incorrectly, because even with |
|
We have |
Merged
via the queue into
rpm-software-management:main
with commit Sep 17, 2024
403bba3
13 of 20 checks passed
ppisar
added a commit
to ppisar/dnf5
that referenced
this pull request
Sep 17, 2024
This patch fixes bugs in sign propagation disovered with
-Wsign-conversion compilar flag and related integer overflows in
dnf5::print_transaction_size_stats(), like this one:
dnf5/main.cpp: In function ‘void dnf5::print_transaction_size_stats(Context&)’:
dnf5/main.cpp:785:29: error: conversion to ‘long long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Werror=sign-conversion]
785 | in_pkgs_size += pkg_size;
| ^~~~~~~~
There were more ways of fixing, but I choose this one:
Use unsigned integers because an integer overflow for signed types is
undefined in C++. I choose unsigned long long int to follow what
get_download_size() and get_install_size() returns. Otherwise, we would
have to add another check whether the value fits into e.g. uint64_t.
Unless we chose uintmax_t.
Explicitly type-cast counters, with a prior range check, on passing
them to libdnf5::cli::utils::units::to_size(). Otherwise, we would
have to add to_size() for unsigned integers to the public API.
(Actuallly to_size() should have been for unsigned integeres from the
very beginning. Package sizes cannot be negative.)
Not printing the transaction size statistics if an irrepresentible
value occurs. Adding more branches to the already complicated code for
the very improbably corner cases does not make much sense.
(I tried also a different implemeation with keeping int64_t as the
counters, but the code was longer and uglier.)
Related: rpm-software-management#1701
ppisar
added a commit
to ppisar/dnf5
that referenced
this pull request
Sep 17, 2024
-Wsign-conversion is by default enabled in Clang with -Wconversion,
but disabled in GCC. Unintented type coercion can misinterpret the
sign as in:
libdnf5-cli/output/transaction_table.cpp:181:97: error: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
181 | P_(" Skipping: {:4} package\n", " Skipping: {:4} packages\n", skips), skips)
| ^~~~~
This would be missed when compiling with GCC, but causing a build
failure with Clang.
This patch brings GCC and Clang compilers in line, both to enable
-Wsign-conversion. However, it disables the warning for SWIG-generated
bindings as the generated code violates the warning many times, like
here for Perl:
bindings/perl5/libdnf5/CMakeFiles/perl5_advisory.dir/advisoryPERL_wrap.cxx:700:36: error: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Werror=sign-conversion]
700 | if (strlen(name) + 1 > (bsz - (r - buff))) return 0;
| ~~~^~~~~~~
Related: rpm-software-management#1701
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 17, 2024
This patch fixes bugs in sign propagation disovered with
-Wsign-conversion compilar flag and related integer overflows in
dnf5::print_transaction_size_stats(), like this one:
dnf5/main.cpp: In function ‘void dnf5::print_transaction_size_stats(Context&)’:
dnf5/main.cpp:785:29: error: conversion to ‘long long unsigned int’ from ‘int64_t’ {aka ‘long int’} may change the sign of the result [-Werror=sign-conversion]
785 | in_pkgs_size += pkg_size;
| ^~~~~~~~
There were more ways of fixing, but I choose this one:
Use unsigned integers because an integer overflow for signed types is
undefined in C++. I choose unsigned long long int to follow what
get_download_size() and get_install_size() returns. Otherwise, we would
have to add another check whether the value fits into e.g. uint64_t.
Unless we chose uintmax_t.
Explicitly type-cast counters, with a prior range check, on passing
them to libdnf5::cli::utils::units::to_size(). Otherwise, we would
have to add to_size() for unsigned integers to the public API.
(Actuallly to_size() should have been for unsigned integeres from the
very beginning. Package sizes cannot be negative.)
Not printing the transaction size statistics if an irrepresentible
value occurs. Adding more branches to the already complicated code for
the very improbably corner cases does not make much sense.
(I tried also a different implemeation with keeping int64_t as the
counters, but the code was longer and uglier.)
Related: #1701
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 17, 2024
-Wsign-conversion is by default enabled in Clang with -Wconversion,
but disabled in GCC. Unintented type coercion can misinterpret the
sign as in:
libdnf5-cli/output/transaction_table.cpp:181:97: error: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Werror=sign-conversion]
181 | P_(" Skipping: {:4} package\n", " Skipping: {:4} packages\n", skips), skips)
| ^~~~~
This would be missed when compiling with GCC, but causing a build
failure with Clang.
This patch brings GCC and Clang compilers in line, both to enable
-Wsign-conversion. However, it disables the warning for SWIG-generated
bindings as the generated code violates the warning many times, like
here for Perl:
bindings/perl5/libdnf5/CMakeFiles/perl5_advisory.dir/advisoryPERL_wrap.cxx:700:36: error: conversion to ‘long unsigned int’ from ‘long int’ may change the sign of the result [-Werror=sign-conversion]
700 | if (strlen(name) + 1 > (bsz - (r - buff))) return 0;
| ~~~^~~~~~~
Related: #1701
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
error: implicit conversion changes signedness: 'const int' to 'unsigned long' [-Werror,-Wsign-conversion]The bug was caused by commit #1696