Skip to content

Remove some implicit conversions #4447

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

Merged
merged 3 commits into from
May 24, 2025

Conversation

dodomorandi
Copy link
Contributor

This fixes some implicit conversions that can cause noise when compiling other projects if conversions warnings (-Wconversion -Wsign-conversion in GCC) are enabled.

The "1" used for the bitshift is treated as int, and this causes an
implicit conversion to `UInt` when performing the logical and.
Explicitly casting the number to `UInt` avoids the warning.
Some indices in `include/fmt/base.h` are expressed as `int` types, which
causes an implicit conversion to a `size_t` when they are actually used
as index. Explicitly casting the value avoids the warning.
The number of bits is used to express the size of a buffer. Using an
`int` causes an implicit conversion warning, let's use a `size_t` which
is the right type for the job.
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.

Please provide a godbolt repro demonstrating the warnings.

@dodomorandi
Copy link
Contributor Author

Please provide a godbolt repro demonstrating the warnings.

The warnings appear when fmt is included in a project with the FetchContent, which obviously needs the compilation of fmt. If the project enables the conversion warnings, they just came out.

The best way to reproduce the warnings is just enable the warnings during the compilation of fmt:

cmake -B build -DCMAKE_CXX_FLAGS="-Wconversion -Wsign-conversion"
cmake --build build -j $(nproc)

@vitaut vitaut merged commit ea985e8 into fmtlib:master May 24, 2025
45 checks passed
@vitaut
Copy link
Contributor

vitaut commented May 24, 2025

We mostly care about -Wall -Wextra -pedantic but these changes are simple enough (and can be further simplified) so I merged them in, thanks.

@dodomorandi dodomorandi deleted the avoid-implicit-conversion branch May 24, 2025 16:28
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.

2 participants