[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/ctype/ctype_utils.h

 // of a function call by inlining them.
 // ------------------------------------------------------
 
-static inline int isalpha(unsigned ch) { return (ch | 32) - 'a' < 26; }
+static inline int isalpha(unsigned char ch) { return (ch | 32) - 'a' < 26; }
 
-static inline int isdigit(unsigned ch) { return (ch - '0') < 10; }
+static inline int isdigit(unsigned char ch) { return (ch - '0') < 10; }
 
-static inline int isalnum(unsigned ch) { return isalpha(ch) || isdigit(ch); }
+static inline int isalnum(unsigned char ch) {
+  return isalpha(ch) || isdigit(ch);
+}
 
-static inline int isgraph(unsigned ch) { return 0x20 < ch && ch < 0x7f; }
+static inline int isgraph(unsigned char ch) { return 0x20 < ch && ch < 0x7f; }
 
-static inline int islower(unsigned ch) { return (ch - 'a') < 26; }
+static inline int islower(unsigned char ch) { return (ch - 'a') < 26; }
 
-static inline int isupper(unsigned ch) { return (ch - 'A') < 26; }
+static inline int isupper(unsigned char ch) { return (ch - 'A') < 26; }
 
 } // namespace internal
 } // namespace __llvm_libc

libc/src/ctype/isalnum.cpp

 
 // 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 internal::isalnum(static_cast<unsigned char>(c));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isalpha.cpp

 
 // 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 internal::isalpha(static_cast<unsigned char>(c));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isblank.cpp

 // 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);
+  const unsigned char ch = static_cast<unsigned char>(c);
   return ch == ' ' || ch == '\t';
 }
 

libc/src/ctype/iscntrl.cpp

 // 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);
+  const unsigned char ch = static_cast<unsigned char>(c);
   return ch < 0x20 || ch == 0x7f;
 }
 

libc/src/ctype/isdigit.cpp

 
 // 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 internal::isdigit(static_cast<unsigned char>(c));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isgraph.cpp

 
 // 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 internal::isgraph(static_cast<unsigned char>(c));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/islower.cpp

 
 // 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 internal::islower(static_cast<unsigned char>(c));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isprint.cpp

 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, isprint, (int c)) {
-  const unsigned ch = c;
+  const unsigned char ch = static_cast<unsigned char>(c);
   return (ch - ' ') < 95;
 }
 

libc/src/ctype/ispunct.cpp

 // 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 char ch = static_cast<unsigned char>(c);
+  return !internal::isalnum(ch) && internal::isgraph(ch);
 }
 
 } // namespace __llvm_libc

libc/src/ctype/isspace.cpp

 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, isspace, (int c)) {
-  const unsigned ch = c;
+  const unsigned char ch = static_cast<unsigned char>(c);
   return ch == ' ' || (ch - '\t') < 5;
 }
 

libc/src/ctype/isupper.cpp

 
 // 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 internal::isupper(static_cast<unsigned char>(c));
+}
 
 } // namespace __llvm_libc

libc/src/ctype/isxdigit.cpp

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

libc/src/ctype/tolower.cpp

 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, tolower, (int c)) {
-  if (internal::isupper(c))
+  if (internal::isupper(static_cast<unsigned char>(c)))
     return c + 'a' - 'A';
   return c;
 }

libc/src/ctype/toupper.cpp

 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
 LLVM_LIBC_FUNCTION(int, toupper, (int c)) {
-  if (internal::islower(c))
+  if (internal::islower(static_cast<unsigned char>(c)))
     return c + 'A' - 'a';
   return c;
 }