Skip to content

Remove some implicit conversions#4447

Merged
vitaut merged 3 commits intofmtlib:masterfrom
dodomorandi:avoid-implicit-conversion
May 24, 2025
Merged

Remove some implicit conversions#4447
vitaut merged 3 commits intofmtlib:masterfrom
dodomorandi:avoid-implicit-conversion

Conversation

@dodomorandi
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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