[libc][NFC] Add explicit casts to ctype functions

[gAlfonso-bit]

Formatted changed lines

[gAlfonso-bit]

This comment was removed by gAlfonso-bit.

[gAlfonso-bit]

@sivachandra ready!

[gAlfonso-bit]

Rebased to LLVM 13's release branch

[gAlfonso-bit]

Rebased to current

[sivachandra]

This patch causes of number of failures in ctype tests. I have not investigated but this change does operations on numbers of different sizes and hence is leading to unexpected results.

[gAlfonso-bit]

Addressed failing tests

[gAlfonso-bit]

Added context to patch

[gAlfonso-bit]

@sivachandra All issues fixed!

[sivachandra]

Can you please rebase this so that I can apply and test it?

[gAlfonso-bit]

Rebased

[gAlfonso-bit]

In D106902#2933549, @sivachandra wrote:

Can you please rebase this so that I can apply and test it?

Done!

[gAlfonso-bit]

@sivachandra All rebased

[sivachandra]

Sorry for the delay, its been hectic for me. I will test this out and submit if everything looks good by the end of the day today (US pacific time).

[sivachandra]

The test for isprint is still failing.

[gAlfonso-bit]

In D106902#2943224, @sivachandra wrote:

The test for isprint is still failing.

Addressed

[sivachandra]

The isprint test is still failing. I have left a comment inline explaining why.

BTW, if you need help with building/testing your patches, let me know and I can give you few instructions. Or, you can lookup the command from the builder logs, from here for example: https://lab.llvm.org/buildbot/#/builders/78/builds/16544

libc/src/ctype/isprint.cpp

19

I think this is still incorrect. For ch - ' ', the operands get promoted to the int type making the predicate evaluate to true for c less than ' '.

[gAlfonso-bit]

Resolved all test failures

[gAlfonso-bit]

In D106902#2950919, @sivachandra wrote:

The isprint test is still failing. I have left a comment inline explaining why.

BTW, if you need help with building/testing your patches, let me know and I can give you few instructions. Or, you can lookup the command from the builder logs, from here for example: https://lab.llvm.org/buildbot/#/builders/78/builds/1654

Addressed!

[gAlfonso-bit]

@sivachandra passed all tests

[sivachandra]

LGTM. I will test the patch one last time and submit it if everything is clean.

libc/src/__support/ctype_utils.h

 namespace internal {
 
 // ------------------------------------------------------
 // Rationale: Since these classification functions are
 // called in other functions, we will avoid the overhead
 // of a function call by inlining them.
 // ------------------------------------------------------
 
-static constexpr int isalpha(unsigned ch) { return (ch | 32) - 'a' < 26; }
+static constexpr bool isalpha(unsigned ch) { return (ch | 32) - 'a' < 26; }
 
-static constexpr int isdigit(unsigned ch) { return (ch - '0') < 10; }
+static constexpr bool isdigit(unsigned ch) { return (ch - '0') < 10; }
 
-static constexpr int isalnum(unsigned ch) { return isalpha(ch) || isdigit(ch); }
+static constexpr bool isalnum(unsigned ch) {
+  return isalpha(ch) || isdigit(ch);
+}
 
-static constexpr int isgraph(unsigned ch) { return 0x20 < ch && ch < 0x7f; }
+static constexpr bool isgraph(unsigned ch) { return 0x20 < ch && ch < 0x7f; }
 
-static constexpr int islower(unsigned ch) { return (ch - 'a') < 26; }
+static constexpr bool islower(unsigned ch) { return (ch - 'a') < 26; }
 
-static constexpr int isupper(unsigned ch) { return (ch - 'A') < 26; }
+static constexpr bool isupper(unsigned ch) { return (ch - 'A') < 26; }
 
-static constexpr int isspace(unsigned ch) {
+static constexpr bool isspace(unsigned ch) {
   return ch == ' ' || (ch - '\t') < 5;
 }
 
 } // namespace internal
 } // namespace __llvm_libc
 
 #endif //  LLVM_LIBC_SRC_SUPPORT_CTYPE_UTILS_H

libc/src/ctype/isalnum.cpp

 #include "src/__support/ctype_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, isalnum, (int c)) { return internal::isalnum(c); }
+LLVM_LIBC_FUNCTION(int, isalnum, (int c)) {
+  return static_cast<int>(internal::isalnum(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isalpha.cpp

 
 #include "src/__support/common.h"
 #include "src/__support/ctype_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, isalpha, (int c)) { return internal::isalpha(c); }
+LLVM_LIBC_FUNCTION(int, isalpha, (int c)) {
+  return static_cast<int>(internal::isalpha(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isascii.cpp

 //===-- Implementation of isascii------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/ctype/isascii.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
-LLVM_LIBC_FUNCTION(int, isascii, (int c)) { return (c & (~0x7f)) == 0; }
+LLVM_LIBC_FUNCTION(int, isascii, (int c)) {
+  return static_cast<int>((c & (~0x7f)) == 0);
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isblank.cpp

 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, isblank, (int c)) {
-  const unsigned char ch = static_cast<char>(c);
-  return ch == ' ' || ch == '\t';
+  const unsigned char ch = static_cast<unsigned char>(c);
+  return static_cast<int>(ch == ' ' || ch == '\t');
 }
 
 } // namespace __llvm_libc

libc/src/ctype/iscntrl.cpp

 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, iscntrl, (int c)) {
-  const unsigned char ch = static_cast<char>(c);
-  return ch < 0x20 || ch == 0x7f;
+  const unsigned char ch = static_cast<unsigned char>(c);
+  return static_cast<int>(ch < 0x20 || ch == 0x7f);
 }
 
 } // namespace __llvm_libc

libc/src/ctype/isdigit.cpp

 //===-- Implementation of isdigit------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "src/ctype/isdigit.h"
 #include "src/__support/common.h"
 #include "src/__support/ctype_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, isdigit, (int c)) { return internal::isdigit(c); }
+LLVM_LIBC_FUNCTION(int, isdigit, (int c)) {
+  return static_cast<int>(internal::isdigit(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isgraph.cpp

 
 #include "src/__support/common.h"
 #include "src/__support/ctype_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, isgraph, (int c)) { return internal::isgraph(c); }
+LLVM_LIBC_FUNCTION(int, isgraph, (int c)) {
+  return static_cast<int>(internal::isgraph(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/islower.cpp

 #include "src/__support/ctype_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, islower, (int c)) { return internal::islower(c); }
+LLVM_LIBC_FUNCTION(int, islower, (int c)) {
+  return static_cast<int>(internal::islower(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isprint.cpp

 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, isprint, (int c)) {
-  const unsigned ch = c;
-  return (ch - ' ') < 95;
+  const unsigned ch = static_cast<unsigned>(c);
+  return static_cast<int>((ch - ' ') < 95);
 }
 
 } // namespace __llvm_libc

libc/src/ctype/ispunct.cpp

 #include "src/__support/common.h"
 #include "src/__support/ctype_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, ispunct, (int c)) {
-  return !internal::isalnum(c) && internal::isgraph(c);
+  const unsigned ch = static_cast<unsigned>(c);
+  return static_cast<int>(!internal::isalnum(ch) && internal::isgraph(ch));
 }
 
 } // namespace __llvm_libc

libc/src/ctype/isspace.cpp

 #include "src/__support/ctype_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, isspace, (int c)) { return internal::isspace(c); }
+LLVM_LIBC_FUNCTION(int, isspace, (int c)) {
+  return static_cast<int>(internal::isspace(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isupper.cpp

 #include "src/__support/ctype_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-LLVM_LIBC_FUNCTION(int, isupper, (int c)) { return internal::isupper(c); }
+LLVM_LIBC_FUNCTION(int, isupper, (int c)) {
+  return static_cast<int>(internal::isupper(static_cast<unsigned>(c)));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isxdigit.cpp

 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, isxdigit, (int c)) {
-  const unsigned ch = c;
-  return internal::isdigit(ch) || (ch | 32) - 'a' < 6;
+  const unsigned ch = static_cast<unsigned>(c);
+  return static_cast<int>(internal::isdigit(ch) || (ch | 32) - 'a' < 6);
 }
 
 } // namespace __llvm_libc

libc/src/ctype/tolower.cpp

 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, tolower, (int c)) {
   if (internal::isupper(c))
-    return c + 'a' - 'A';
+    return c + ('a' - 'A');
   return c;
 }
 
 } // namespace __llvm_libc

libc/src/ctype/toupper.cpp

 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, toupper, (int c)) {
   if (internal::islower(c))
-    return c + 'A' - 'a';
+    return c - ('a' - 'A');
   return c;
 }
 
 } // namespace __llvm_libc