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

[libc] Fix recently introduced integer-type warnings #125864

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

statham-arm
Copy link
Collaborator

These warnings all come up in code modified by one of my two recent commits c06d0ff and b53da77, and all relate to implicit integer type conversion. In a build of ours with strict compile options two of them became errors. Even without that problem, it's worth fixing them to reduce noise that might hide a more serious warning.

These warnings all come up in code modified by one of my two recent
commits c06d0ff and b53da77, and all relate to
implicit integer type conversion. In a build of ours with strict
compile options two of them became errors. Even without that problem,
it's worth fixing them to reduce noise that might hide a more serious
warning.
@llvmbot
Copy link
Member

llvmbot commented Feb 5, 2025

@llvm/pr-subscribers-libc

Author: Simon Tatham (statham-arm)

Changes

These warnings all come up in code modified by one of my two recent commits c06d0ff and b53da77, and all relate to implicit integer type conversion. In a build of ours with strict compile options two of them became errors. Even without that problem, it's worth fixing them to reduce noise that might hide a more serious warning.


Full diff: https://github.com/llvm/llvm-project/pull/125864.diff

3 Files Affected:

  • (modified) libc/src/__support/FPUtil/dyadic_float.h (+1-1)
  • (modified) libc/src/__support/integer_to_string.h (+1-1)
  • (modified) libc/src/stdio/printf_core/float_dec_converter_limited.h (+4-4)
diff --git a/libc/src/__support/FPUtil/dyadic_float.h b/libc/src/__support/FPUtil/dyadic_float.h
index 586690f3a32c19..16b690be08612b 100644
--- a/libc/src/__support/FPUtil/dyadic_float.h
+++ b/libc/src/__support/FPUtil/dyadic_float.h
@@ -604,7 +604,7 @@ approx_reciprocal(const DyadicFloat<Bits> &a) {
   // of correct bits in x' is double the number in x.
 
   // An initial approximation to the reciprocal
-  DyadicFloat<Bits> x(Sign::POS, -32 - a.exponent - Bits,
+  DyadicFloat<Bits> x(Sign::POS, -32 - a.exponent - int(Bits),
                       uint64_t(0xFFFFFFFFFFFFFFFF) /
                           static_cast<uint64_t>(a.mantissa >> (Bits - 32)));
 
diff --git a/libc/src/__support/integer_to_string.h b/libc/src/__support/integer_to_string.h
index 57fd8be9e6e8bc..13dd83e9ed2b11 100644
--- a/libc/src/__support/integer_to_string.h
+++ b/libc/src/__support/integer_to_string.h
@@ -231,7 +231,7 @@ extract_decimal_digit(T &value) {
   // Having multiplied it by 6, add the lowest half-word, and then reduce mod
   // 10 by normal integer division to finish.
   acc_remainder += value.val[0] & HALFWORD_MASK;
-  uint8_t digit = acc_remainder % 10;
+  uint8_t digit = static_cast<uint8_t>(acc_remainder % 10);
 
   // Now we have the output digit. Subtract it from the input value, and shift
   // right to divide by 2.
diff --git a/libc/src/stdio/printf_core/float_dec_converter_limited.h b/libc/src/stdio/printf_core/float_dec_converter_limited.h
index a98e6cdaa0db7e..95516aa02eaa6a 100644
--- a/libc/src/stdio/printf_core/float_dec_converter_limited.h
+++ b/libc/src/stdio/printf_core/float_dec_converter_limited.h
@@ -113,7 +113,7 @@ struct DigitsOutput {
 // enough that for any binary exponent in the range of float128 it will give
 // the correct value of floor(log10(2^n)).
 LIBC_INLINE int estimate_log10(int exponent_of_2) {
-  return (exponent_of_2 * 1292913986LL) >> 32;
+  return static_cast<int>((exponent_of_2 * 1292913986LL) >> 32);
 }
 
 // Calculate the actual digits of a decimal representation of an FP number.
@@ -189,7 +189,7 @@ DigitsOutput decimal_digits(DigitsInput input, int precision, bool e_mode) {
   // overflow _after_ the multiplication and retry. So if even the smaller
   // number of possible output digits is too many, we might as well change our
   // mind right now and switch into E mode.
-  if (log10_input_max - log10_low_digit + 1 > MAX_DIGITS) {
+  if (log10_input_max - log10_low_digit + 1 > int(MAX_DIGITS)) {
     precision = MAX_DIGITS;
     e_mode = true;
     log10_low_digit = log10_input_min + 1 - precision;
@@ -362,8 +362,8 @@ DigitsOutput decimal_digits(DigitsInput input, int precision, bool e_mode) {
       // we made it from and doing the decimal conversion all over again.)
       for (size_t i = output.ndigits; i-- > 0;) {
         if (output.digits[i] != '9') {
-          output.digits[i] = internal::int_to_b36_char(
-              internal::b36_char_to_int(output.digits[i]) + 1);
+          output.digits[i] = static_cast<char>(internal::int_to_b36_char(
+              internal::b36_char_to_int(output.digits[i]) + 1));
           break;
         } else {
           output.digits[i] = '0';

@statham-arm
Copy link
Collaborator Author

Some examples of the warnings:

.../libc/src/stdio/printf_core/float_dec_converter_limited.h:365:30: error: implicit conversion loses integer precision: 'int' to 'char' [-Werror,-Wimplicit-int-conversion]
  365 |           output.digits[i] = internal::int_to_b36_char(
      |                            ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~
  366 |               internal::b36_char_to_int(output.digits[i]) + 1);
      |               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

but this is in fact fine because b36_int_to_char always returns something that fits in a char, even though it's typed as int. So I've wrapped it in static_cast<int> to force the compiler to truncate it without complaining, knowing that the truncation won't actually introduce a problem.

.../libc/src/stdio/printf_core/float_dec_converter_limited.h:192:45: error: comparison of integers of different signs: 'int' and 'const unsigned int' [-Werror,-Wsign-compare]
  192 |   if (log10_input_max - log10_low_digit + 1 > MAX_DIGITS) {
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~

but in fact MAX_DIGITS also fits in an int, and the compiler can perfectly well check that at compile time since it's constexpr. So in this case I've wrapped it in a less verbose constructor call, int(MAX_DIGITS), in the hope that if MAX_DIGITS were accidentally redefined too large later, some compiler might still check it.

Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me to avoid the warnings, but worth giving some time for a LLVM libc reviewer to check over it.

@statham-arm statham-arm merged commit 7ef33e6 into llvm:main Feb 6, 2025
16 checks passed
@statham-arm statham-arm deleted the libc-printf-warning-fixes branch February 6, 2025 09:15
Icohedron pushed a commit to Icohedron/llvm-project that referenced this pull request Feb 11, 2025
These warnings all come up in code modified by one of my two recent
commits c06d0ff and b53da77, and all relate to implicit
integer type conversion. In a build of ours with strict compile options
two of them became errors. Even without that problem, it's worth fixing
them to reduce noise that might hide a more serious warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants