[libc] Add missing casts in StrtolTest

[frobtech]

The a0c4f85 change replaced the
local int_to_b36_char function returning char with uses of the
__support function of the same name that returns int. The uses
of the old local function lacked the casts that all other uses of
the shared function of the same name had. Add them.

[llvmbot]

@llvm/pr-subscribers-libc

Author: Roland McGrath (frobtech)

Changes

The a0c4f85 change replaced the
local int_to_b36_char function returning char with uses of the
__support function of the same name that returns int. The uses
of the old local function lacked the casts that all other uses of
the shared function of the same name had. Add them.

---

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

1 Files Affected:

• (modified) libc/test/src/stdlib/StrtolTest.h (+12-12)

diff --git a/libc/test/src/stdlib/StrtolTest.h b/libc/test/src/stdlib/StrtolTest.h
index 6cfaddcbedeb6a..ed302f14d03ef4 100644
--- a/libc/test/src/stdlib/StrtolTest.h
+++ b/libc/test/src/stdlib/StrtolTest.h
@@ -200,8 +200,8 @@ struct StrtoTest : public LIBC_NAMESPACE::testing::Test {
     char small_string[4] = {'\0', '\0', '\0', '\0'};
     for (int base = 2; base <= 36; ++base) {
       for (int first_digit = 0; first_digit <= 36; ++first_digit) {
-        small_string[0] =
-            LIBC_NAMESPACE::internal::int_to_b36_char(first_digit);
+        small_string[0] = static_cast<char>(
+            LIBC_NAMESPACE::internal::int_to_b36_char(first_digit));
         if (first_digit < base) {
           LIBC_NAMESPACE::libc_errno = 0;
           ASSERT_EQ(func(small_string, nullptr, base),
@@ -217,11 +217,11 @@ struct StrtoTest : public LIBC_NAMESPACE::testing::Test {
 
     for (int base = 2; base <= 36; ++base) {
       for (int first_digit = 0; first_digit <= 36; ++first_digit) {
-        small_string[0] =
-            LIBC_NAMESPACE::internal::int_to_b36_char(first_digit);
+        small_string[0] = static_cast<char>(
+            LIBC_NAMESPACE::internal::int_to_b36_char(first_digit));
         for (int second_digit = 0; second_digit <= 36; ++second_digit) {
-          small_string[1] =
-              LIBC_NAMESPACE::internal::int_to_b36_char(second_digit);
+          small_string[1] = static_cast<char>(
+              LIBC_NAMESPACE::internal::int_to_b36_char(second_digit));
           if (first_digit < base && second_digit < base) {
             LIBC_NAMESPACE::libc_errno = 0;
             ASSERT_EQ(
@@ -244,14 +244,14 @@ struct StrtoTest : public LIBC_NAMESPACE::testing::Test {
 
     for (int base = 2; base <= 36; ++base) {
       for (int first_digit = 0; first_digit <= 36; ++first_digit) {
-        small_string[0] =
-            LIBC_NAMESPACE::internal::int_to_b36_char(first_digit);
+        small_string[0] = static_cast<char>(
+            LIBC_NAMESPACE::internal::int_to_b36_char(first_digit));
         for (int second_digit = 0; second_digit <= 36; ++second_digit) {
-          small_string[1] =
-              LIBC_NAMESPACE::internal::int_to_b36_char(second_digit);
+          small_string[1] = static_cast<char>(
+              LIBC_NAMESPACE::internal::int_to_b36_char(second_digit));
           for (int third_digit = 0; third_digit <= limit; ++third_digit) {
-            small_string[2] =
-                LIBC_NAMESPACE::internal::int_to_b36_char(third_digit);
+            small_string[2] = static_cast<char>(
+                LIBC_NAMESPACE::internal::int_to_b36_char(third_digit));
 
             if (first_digit < base && second_digit < base &&
                 third_digit < base) {

[nickdesaulniers]

Was this exposed by a truncation warning set downstream that we should perhaps be setting in our cmakelists upstream?

[frobtech]

Was this exposed by a truncation warning set downstream that we should perhaps be setting in our cmakelists upstream?

The Fuchsia build uses fairly strict warnings. The error cites -Wimplicit-int-conversion though we don't enable that warning specifically. I'm not sure which warning switch we're using enables that.

[nickdesaulniers]

I'm not sure which warning switch we're using enables that.

LLVM has a neat utility called diagtool that can be used to quickly discover whether a particular clang diagnostic command line flag is part of another diagnostic "group." (Every time you add a new clang diagnostic command line flag, an existing "change detector" test trips; it detects changes by having a golden reference for the output of diagtool tree.)

In this case, the group hierarchy for -Wimplicit-int-conversion looks like:

-Wnon-gcc
...
  -Wconversion
...
    -Wimplicit-int-conversion

(so -Wimplicit-int-conversion can either be enable explicitly via -Wimplicit-int-conversion, or implicitly via -Wconversion or -Wnon-gcc. Of note: it's not part of -Wall or -Wextra).

It's probably worthwhile for us to set at least -Wimplicit-int-conversion upstream in our cmake. -Wconversion itself is a large group, and might be quite noisy.

[nickdesaulniers]

err libc/cmake/modules/LLVMLibCCompileOptionRules.cmakeL161 sets -Wconversion...
ah, I bet the tests are missing this same flag.

[nickdesaulniers]

err libc/cmake/modules/LLVMLibCCompileOptionRules.cmakeL237 sets -Wconversion, but it's commented out! Let me do some git archaeology to better understand why.

[nickdesaulniers]

hmmm d0c2c0a #81917 that TODO never got filed FWICT...heading out the door for lunch, but will file one when I get back.

[nickdesaulniers]

Filed #119281.