[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 ↗

(On Diff #366672)

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/string/aarch64/memcmp.cpp

static int memcmp_aarch64(const char *lhs, const char *rhs, size_t count) {
   if (count < 64) // [33, 63]
     return ThreeWayCompare<Tail<_32>>(lhs, rhs, count);
   // [64, 127]
   return ThreeWayCompare<Skip<32>::Then<Loop<_16>>>(lhs, rhs, count);
 }
 
 LLVM_LIBC_FUNCTION(int, memcmp,
                    (const void *lhs, const void *rhs, size_t count)) {
-  return memcmp_aarch64(reinterpret_cast<const char *>(lhs),
-                        reinterpret_cast<const char *>(rhs), count);
+  return memcmp_aarch64(static_cast<const char *>(lhs),
+                        static_cast<const char *>(rhs), count);
 }
 
 } // namespace __llvm_libc

libc/src/string/aarch64/memcpy.cpp

static void memcpy_aarch64(char *__restrict dst, const char *__restrict src,
   if (count < 128)
     return Copy<HeadTail<_64>>(dst, src, count);
   return Copy<Align<_16, Arg::Src>::Then<Loop<_64>>>(dst, src, count);
 }
 
 LLVM_LIBC_FUNCTION(void *, memcpy,
                    (void *__restrict dst, const void *__restrict src,
                     size_t size)) {
-  memcpy_aarch64(reinterpret_cast<char *>(dst),
-                 reinterpret_cast<const char *>(src), size);
+  memcpy_aarch64(static_cast<char *>(dst), static_cast<const char *>(src),
+                 size);
   return dst;
 }
 
 } // namespace __llvm_libc

libc/src/string/bzero.cpp

 //===-- Implementation of bzero -------------------------------------------===//
 //
 // 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/string/bzero.h"
 #include "src/__support/common.h"
 #include "src/string/memory_utils/memset_utils.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(void, bzero, (void *ptr, size_t count)) {
-  GeneralPurposeMemset(reinterpret_cast<char *>(ptr), 0, count);
+  GeneralPurposeMemset(static_cast<char *>(ptr), 0, count);
 }
 
 } // namespace __llvm_libc

libc/src/string/memchr.cpp

 
 #include "src/__support/common.h"
 #include <stddef.h>
 
 namespace __llvm_libc {
 
 // TODO: Look at performance benefits of comparing words.
 LLVM_LIBC_FUNCTION(void *, memchr, (const void *src, int c, size_t n)) {
-  return internal::find_first_character(
-      reinterpret_cast<const unsigned char *>(src), c, n);
+  return internal::find_first_character(static_cast<const unsigned char *>(src),
+                                        static_cast<unsigned char>(c), n);
 }
 
 } // namespace __llvm_libc

libc/src/string/memcmp.cpp

 #include "src/__support/common.h"
 #include <stddef.h> // size_t
 
 namespace __llvm_libc {
 
 // TODO: It is a simple implementation, an optimized version is preparing.
 LLVM_LIBC_FUNCTION(int, memcmp,
                    (const void *lhs, const void *rhs, size_t count)) {
-  const unsigned char *_lhs = reinterpret_cast<const unsigned char *>(lhs);
-  const unsigned char *_rhs = reinterpret_cast<const unsigned char *>(rhs);
+  const unsigned char *_lhs = static_cast<const unsigned char *>(lhs);
+  const unsigned char *_rhs = static_cast<const unsigned char *>(rhs);
   for (size_t i = 0; i < count; ++i)
     if (_lhs[i] != _rhs[i])
       return _lhs[i] - _rhs[i];
   // count is 0 or _lhs and _rhs are the same.
   return 0;
 }
 
 } // namespace __llvm_libc

libc/src/string/memcpy.cpp

static void memcpy_impl(char *__restrict dst, const char *__restrict src,
   if (count < 128)
     return Copy<HeadTail<_64>>(dst, src, count);
   return Copy<Align<_32, Arg::Src>::Then<Loop<_32>>>(dst, src, count);
 }
 
 LLVM_LIBC_FUNCTION(void *, memcpy,
                    (void *__restrict dst, const void *__restrict src,
                     size_t size)) {
-  memcpy_impl(reinterpret_cast<char *>(dst),
-              reinterpret_cast<const char *>(src), size);
+  memcpy_impl(static_cast<char *>(dst), static_cast<const char *>(src), size);
   return dst;
 }
 
 } // namespace __llvm_libc

libc/src/string/memmove.cpp

 
 #include "src/__support/common.h"
 #include "src/__support/integer_operations.h"
 #include "src/string/memcpy.h"
 #include <stddef.h> // size_t, ptrdiff_t
 
 namespace __llvm_libc {
 
-static inline void move_byte_forward(char *dest_m, const char *src_m,
-                                     size_t count) {
-  for (size_t offset = 0; count; --count, ++offset)
+static inline void move_byte_forward(unsigned char *dest_m,
+                                     const unsigned char *src_m, size_t count) {
+  for (size_t offset = 0; offset != count; ++offset)
     dest_m[offset] = src_m[offset];
 }
 
-static inline void move_byte_backward(char *dest_m, const char *src_m,
+static inline void move_byte_backward(unsigned char *dest_m,
+                                      const unsigned char *src_m,
                                       size_t count) {
-  for (size_t offset = count - 1; count; --count, --offset)
-    dest_m[offset] = src_m[offset];
+  for (size_t offset = count; offset != 0; --offset)
+    dest_m[offset - 1] = src_m[offset - 1];
 }
 
 LLVM_LIBC_FUNCTION(void *, memmove,
                    (void *dest, const void *src, size_t count)) {
-  char *dest_c = reinterpret_cast<char *>(dest);
-  const char *src_c = reinterpret_cast<const char *>(src);
+  unsigned char *dest_c = static_cast<unsigned char *>(dest);
+  const unsigned char *src_c = static_cast<const unsigned char *>(src);
 
   // If the distance between src_c and dest_c is equal to or greater
   // than count (integerAbs(src_c - dest_c) >= count), they would not overlap.
   // e.g.   greater     equal       overlapping
   //        [12345678]  [12345678]  [12345678]
   // src_c: [_ab_____]  [_ab_____]  [_ab_____]
   // dest_c:[_____yz_]  [___yz___]  [__yz____]
 
   // Use memcpy if src_c and dest_c do not overlap.
   if (__llvm_libc::integerAbs(src_c - dest_c) >= static_cast<ptrdiff_t>(count))
-    return __llvm_libc::memcpy(dest_c, src_c, count);
+    return __llvm_libc::memcpy(dest, src, count);
 
   // Overlap cases.
   // If dest_c starts before src_c (dest_c < src_c), copy forward(pointer add 1)
   // from beginning to end.
   // If dest_c starts after src_c (dest_c > src_c), copy backward(pointer add
   // -1) from end to beginning.
-  // If dest_c and src_c start at the same address (dest_c == src_c),
-  // just return dest.
+  // It is extremely unlikely that dest and src are equal, and even if they
+  // were, nothing of consequence would happen
   // e.g.    forward      backward
   //             *-->        <--*
   // src_c : [___abcde_]  [_abcde___]
   // dest_c: [_abc--___]  [___--cde_]
 
   // TODO: Optimize `move_byte_xxx(...)` functions.
   if (dest_c < src_c)
     move_byte_forward(dest_c, src_c, count);
-  if (dest_c > src_c)
+  else
     move_byte_backward(dest_c, src_c, count);
   return dest;
 }
 
 } // namespace __llvm_libc

libc/src/string/memory_utils/elements.h

static bool Equals(const char *lhs, const char *rhs) {
     }
     return true;
   }
 
   static int ThreeWayCompare(const char *lhs, const char *rhs) {
     for (size_t i = 0; i < ElementCount; ++i) {
       const size_t offset = i * Element::kSize;
       // We make the assumption that 'Equals' si cheaper than 'ThreeWayCompare'.
-      if (Element::Equals(lhs + offset, rhs + offset))
-        continue;
+      if (!Element::Equals(lhs + offset, rhs + offset))
         return Element::ThreeWayCompare(lhs + offset, rhs + offset);
     }
     return 0;
   }
 
   static void SplatSet(char *dst, const unsigned char value) {
     for (size_t i = 0; i < ElementCount; ++i) {
       const size_t offset = i * Element::kSize;
       Element::SplatSet(dst + offset, value);
template <typename Head, typename... Tail> struct Chained<Head, Tail...> {
 
   static bool Equals(const char *lhs, const char *rhs) {
     if (!__llvm_libc::Equals<Head>(lhs, rhs))
       return false;
     return Chained<Tail...>::Equals(lhs + Head::kSize, rhs + Head::kSize);
   }
 
   static int ThreeWayCompare(const char *lhs, const char *rhs) {
-    if (__llvm_libc::Equals<Head>(lhs, rhs))
-      return Chained<Tail...>::ThreeWayCompare(lhs + Head::kSize,
-                                               rhs + Head::kSize);
-    return __llvm_libc::ThreeWayCompare<Head>(lhs, rhs);
+    if (!__llvm_libc::Equals<Head>(lhs, rhs))
+      return __llvm_libc::ThreeWayCompare<Head>(lhs, rhs);
   }
 
   static void SplatSet(char *dst, const unsigned char value) {
     Chained<Tail...>::SplatSet(dst + Head::kSize, value);
     __llvm_libc::SplatSet<Head>(dst, value);
   }
 };
 

libc/src/string/memrchr.cpp

 //===-- Implementation of memrchr -----------------------------------------===//
 //
 // 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/string/memrchr.h"
 #include "src/__support/common.h"
 #include <stddef.h>
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(void *, memrchr, (const void *src, int c, size_t n)) {
-  const unsigned char *str = reinterpret_cast<const unsigned char *>(src);
-  const unsigned char ch = c;
+  const unsigned char *str = static_cast<const unsigned char *>(src);
+  const unsigned char ch = static_cast<unsigned char>(c);
   for (; n != 0; --n) {
-    const unsigned char *s = str + n - 1;
-    if (*s == ch)
-      return const_cast<unsigned char *>(s);
+    if (*(--str) == ch)
+      return const_cast<unsigned char *>(str);
   }
   return nullptr;
 }
 
 } // namespace __llvm_libc

libc/src/string/memset.cpp

 //===-- Implementation of memset ------------------------------------------===//
 //
 // 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/string/memset.h"
 #include "src/__support/common.h"
 #include "src/string/memory_utils/memset_utils.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(void *, memset, (void *dst, int value, size_t count)) {
-  GeneralPurposeMemset(reinterpret_cast<char *>(dst),
+  GeneralPurposeMemset(static_cast<char *>(dst),
                        static_cast<unsigned char>(value), count);
   return dst;
 }
 
 } // namespace __llvm_libc

libc/src/string/strchr.cpp

 //===-- Implementation of strchr ------------------------------------------===//
 //
 // 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/string/strchr.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Look at performance benefits of comparing words.
 LLVM_LIBC_FUNCTION(char *, strchr, (const char *src, int c)) {
-  unsigned char *str =
-      const_cast<unsigned char *>(reinterpret_cast<const unsigned char *>(src));
-  const unsigned char ch = c;
-  for (; *str && *str != ch; ++str)
-    ;
-  return *str == ch ? reinterpret_cast<char *>(str) : nullptr;
+  const char ch = static_cast<char>(c);
+  for (; *src != ch; ++src)
+    if (*src == '\0')
+      return nullptr;
+
+  return const_cast<char *>(src);
 }
 
 } // namespace __llvm_libc

libc/src/string/strcpy.cpp

 #include "src/string/string_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strcpy,
                    (char *__restrict dest, const char *__restrict src)) {
-  return reinterpret_cast<char *>(
+  return static_cast<char *>(
       __llvm_libc::memcpy(dest, src, internal::string_length(src) + 1));
 }
 
 } // namespace __llvm_libc

libc/src/string/strrchr.cpp

 //===-- Implementation of strrchr------------------------------------------===//
 //
 // 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/string/strrchr.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 LLVM_LIBC_FUNCTION(char *, strrchr, (const char *src, int c)) {
-  const char ch = c;
+  const char ch = static_cast<char>(c);
   char *last_occurrence = nullptr;
   do {
     if (*src == ch)
       last_occurrence = const_cast<char *>(src);
-  } while (*src++);
+  } while (*src++ != '\0');
   return last_occurrence;
 }
 
 } // namespace __llvm_libc

libc/src/string/strstr.cpp

 #include "src/__support/common.h"
 #include <stddef.h>
 
 namespace __llvm_libc {
 
 // TODO: This is a simple brute force implementation. This can be
 // improved upon using well known string matching algorithms.
 LLVM_LIBC_FUNCTION(char *, strstr, (const char *haystack, const char *needle)) {
-  for (size_t i = 0; haystack[i]; ++i) {
+  for (size_t i = 0; haystack[i] != '\0'; ++i) {
     size_t j;
-    for (j = 0; haystack[i + j] && haystack[i + j] == needle[j]; ++j)
+    for (j = 0; haystack[i + j] != '\0' && haystack[i + j] == needle[j]; ++j)
       ;
-    if (!needle[j])
+    if (needle[j] == '\0')
       return const_cast<char *>(haystack + i);
   }
   return nullptr;
 }
 
 } // namespace __llvm_libc

libc/src/string/x86_64/memcpy.cpp

if (count <= kRepMovsBSize)
     return Copy<Align<_32, Arg::Dst>::Then<Loop<LoopBlockSize>>>(dst, src,
                                                                  count);
   return CopyRepMovsb(dst, src, count);
 }
 
 LLVM_LIBC_FUNCTION(void *, memcpy,
                    (void *__restrict dst, const void *__restrict src,
                     size_t size)) {
-  memcpy_x86(reinterpret_cast<char *>(dst), reinterpret_cast<const char *>(src),
-             size);
+  memcpy_x86(static_cast<char *>(dst), static_cast<const char *>(src), size);
   return dst;
 }
 
 } // namespace __llvm_libc