[libc] Implements isdigit and isalnum.

[sivachandra]

Overall LGTM. To avoid call instructions to other classification functions, for example isalnum calls isdigit and or isalpha does it make sense to have a library of static inline implementations in a header file. Then, the implementation functions will call the static inline functions, but the call overhead is avoided because of them being static inline. WDYT?

[cgyurgyik]

In D84893#2183796, @sivachandra wrote:

Overall LGTM. To avoid call instructions to other classification functions, for example isalnum calls isdigit and or isalpha does it make sense to have a library of static inline implementations in a header file. Then, the implementation functions will call the static inline functions, but the call overhead is avoided because of them being static inline. WDYT?

Inlining is good, but comes at a price of abstraction.

1. In this case, we probably want to use the static inline definition as the one provided for isdigit as well, right? Even though, its two lines of code, I think its better to avoid defining it twice.

2. I'm assuming we want to do this solely for ctype functions that are used in other functions, correct? It might be confusing initially for someone looking at the library, but I don't see a point in having an inlined function that's not called elsewhere.

[cgyurgyik]

To demonstrate my thoughts, adds inlined versions of functions isalpha, isdigit in a utility header.

[sivachandra]

In D84893#2184401, @cgyurgyik wrote:

In D84893#2183796, @sivachandra wrote:

Overall LGTM. To avoid call instructions to other classification functions, for example isalnum calls isdigit and or isalpha does it make sense to have a library of static inline implementations in a header file. Then, the implementation functions will call the static inline functions, but the call overhead is avoided because of them being static inline. WDYT?

Inlining is good, but comes at a price of abstraction.

1. In this case, we probably want to use the static inline definition as the one provided for isdigit as well, right? Even though, its two lines of code, I think its better to avoid defining it twice.

I am not sure I understand what you mean by defining twice.

2. I'm assuming we want to do this solely for ctype functions that are used in other functions, correct? It might be confusing initially for someone looking at the library, but I don't see a point in having an inlined function that's not called elsewhere.

For example, inlined_isdigit is called at two places.

Anyways, the point is to make the functions as standalone as possible. For example, we want to keep isalnum independent without requiring isdigit and isalpha. This might not be possible in general for all libc functions, but ctype functions are simple enough that we can do it.

libc/src/ctype/ctype_utils.h

21

Instead of the inlined_ prefix, may be put them in a nested namespace internal?

[cgyurgyik]

In D84893#2185074, @sivachandra wrote:

In D84893#2184401, @cgyurgyik wrote:

In D84893#2183796, @sivachandra wrote:

Overall LGTM. To avoid call instructions to other classification functions, for example isalnum calls isdigit and or isalpha does it make sense to have a library of static inline implementations in a header file. Then, the implementation functions will call the static inline functions, but the call overhead is avoided because of them being static inline. WDYT?

Inlining is good, but comes at a price of abstraction.

1. In this case, we probably want to use the static inline definition as the one provided for isdigit as well, right? Even though, its two lines of code, I think its better to avoid defining it twice.

I am not sure I understand what you mean by defining twice.

2. I'm assuming we want to do this solely for ctype functions that are used in other functions, correct? It might be confusing initially for someone looking at the library, but I don't see a point in having an inlined function that's not called elsewhere.

For example, inlined_isdigit is called at two places.

Anyways, the point is to make the functions as standalone as possible. For example, we want to keep isalnum independent without requiring isdigit and isalpha. This might not be possible in general for all libc functions, but ctype functions are simple enough that we can do it.

I meant that the LLVM_LIBC_ENTRYPOINT(isdigit) should just call internal::isdigit. I think we're on the same page.

libc/config/linux/aarch64/entrypoints.txt

 set(TARGET_LIBC_ENTRYPOINTS
     # ctype.h entrypoints
+    libc.src.ctype.isalnum
     libc.src.ctype.isalpha
+    libc.src.ctype.isdigit
     
     # errno.h entrypoints
     libc.src.errno.__errno_location
 
     # string.h entrypoints
     libc.src.string.bzero
     libc.src.string.memchr
     libc.src.string.memcpy

libc/config/linux/api.td

def AssertAPI : PublicAPI<"assert.h"> {
   let Macros = [
     AssertMacro,
     StaticAssertMacro,
   ];
 }
 
 def CTypeAPI : PublicAPI<"ctype.h"> {
   let Functions = [
+    "isalnum",
     "isalpha",
+    "isdigit",
   ];
 }
 
 def MathErrHandlingMacro : MacroDef<"math_errhandling"> {
   let Defn = [{
     #ifndef math_errhandling
     #ifdef __FAST_MATH__
     #define math_errhandling 0

libc/config/linux/x86_64/entrypoints.txt

 set(TARGET_LIBC_ENTRYPOINTS
     # assert.h entrypoints
     libc.src.assert.__assert_fail
     
     # ctype.h entrypoints
+    libc.src.ctype.isalnum
     libc.src.ctype.isalpha
+    libc.src.ctype.isdigit
 
     # errno.h entrypoints
     libc.src.errno.__errno_location
 
     # signal.h entrypoints
     libc.src.signal.raise
     libc.src.signal.sigaction
     libc.src.signal.sigdelset

libc/spec/stdc.td

def StdC : StandardSpec<"stdc"> {
   
   HeaderSpec CType = HeaderSpec<
       "ctype.h",
       [], // Macros
       [], // Types
       [], // Enumerations
       [
           FunctionSpec<
+              "isalnum",
+              RetValSpec<IntType>,
+              [ArgSpec<IntType>]
+          >,
+          FunctionSpec<
               "isalpha",
               RetValSpec<IntType>,
               [ArgSpec<IntType>]
           >,
+          FunctionSpec<
+              "isdigit",
+              RetValSpec<IntType>,
+              [ArgSpec<IntType>]
+          >,
       ]
   >;
      
   HeaderSpec String = HeaderSpec<
       "string.h",
       [
           Macro<"NULL">,
       ],

libc/src/ctype/CMakeLists.txt

+add_header_library(
+  ctype_utils
+  HDRS
+    ctype_utils.h
+)
+
+add_entrypoint_object(
+  isalnum
+  SRCS
+    isalnum.cpp
+  HDRS
+    isalnum.h
+  DEPENDS
+    .ctype_utils
+)
+
 add_entrypoint_object(
   isalpha
   SRCS
     isalpha.cpp
   HDRS
     isalpha.h
+  DEPENDS
+    .ctype_utils
+)
+
+add_entrypoint_object(
+  isdigit
+  SRCS
+    isdigit.cpp
+  HDRS
+    isdigit.h
+  DEPENDS
+    .ctype_utils
 )

libc/src/ctype/ctype_utils.h

+//===-- Collection of utils for implementing ctype functions-------*-C++-*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_CTYPE_CTYPE_UTILS_H
+#define LLVM_LIBC_SRC_CTYPE_CTYPE_UTILS_H
+
+namespace __llvm_libc {
+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 inline int isdigit(int c) {
+  const unsigned ch = c;
+  return (ch - '0') < 10;
+}
+
+static inline int isalpha(int c) {
+  const unsigned ch = c;
+  return (ch | 32) - 'a' < 26;
+}
+
+} // namespace internal
+} // namespace __llvm_libc
+
+#endif //  LLVM_LIBC_SRC_CTYPE_CTYPE_UTILS_H

libc/src/ctype/isalnum.h

+//===-- Implementation header for isalnum -------------------------*-C++-*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_CTYPE_ISALNUM_H
+#define LLVM_LIBC_SRC_CTYPE_ISALNUM_H
+
+namespace __llvm_libc {
+
+int isalnum(int c);
+
+} // namespace __llvm_libc
+
+#endif //  LLVM_LIBC_SRC_CTYPE_ISALNUM_H

libc/src/ctype/isalnum.cpp

-//===-- Implementation of isalpha------------------------------------------===//
+//===-- Implementation of isalnum------------------------------------------===//
 //
 // 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/isalpha.h"
+#include "src/ctype/isalnum.h"
+#include "src/ctype/ctype_utils.h"
 
 #include "src/__support/common.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-int LLVM_LIBC_ENTRYPOINT(isalpha)(int c) {
-  const unsigned ch = c;
-  return (ch | 32) - 'a' < 26;
+int LLVM_LIBC_ENTRYPOINT(isalnum)(int c) {
+  return internal::isalpha(c) || internal::isdigit(c);
 }
 
 } // namespace __llvm_libc

libc/src/ctype/isalpha.cpp

 //===-- Implementation of isalpha------------------------------------------===//
 //
 // 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/isalpha.h"
 
 #include "src/__support/common.h"
+#include "src/ctype/ctype_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-int LLVM_LIBC_ENTRYPOINT(isalpha)(int c) {
-  const unsigned ch = c;
-  return (ch | 32) - 'a' < 26;
-}
+int LLVM_LIBC_ENTRYPOINT(isalpha)(int c) { return internal::isalpha(c); }
 
 } // namespace __llvm_libc

libc/src/ctype/isdigit.h

+//===-- Implementation header for isdigit -------------------------*-C++-*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_CTYPE_ISDIGIT_H
+#define LLVM_LIBC_SRC_CTYPE_ISDIGIT_H
+
+namespace __llvm_libc {
+
+int isdigit(int c);
+
+} // namespace __llvm_libc
+
+#endif //  LLVM_LIBC_SRC_CTYPE_ISDIGIT_H

libc/src/ctype/isdigit.cpp

-//===-- Implementation of isalpha------------------------------------------===//
+//===-- 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/isalpha.h"
-
+#include "src/ctype/isdigit.h"
 #include "src/__support/common.h"
+#include "src/ctype/ctype_utils.h"
 
 namespace __llvm_libc {
 
 // TODO: Currently restricted to default locale.
 // These should be extended using locale information.
-int LLVM_LIBC_ENTRYPOINT(isalpha)(int c) {
-  const unsigned ch = c;
-  return (ch | 32) - 'a' < 26;
-}
+int LLVM_LIBC_ENTRYPOINT(isdigit)(int c) { return internal::isdigit(c); }
 
 } // namespace __llvm_libc

libc/test/src/ctype/CMakeLists.txt

 add_libc_testsuite(libc_ctype_unittests)
 
 add_libc_unittest(
+  isalnum
+  SUITE
+    libc_ctype_unittests
+  SRCS
+    isalnum_test.cpp
+  DEPENDS
+    libc.src.ctype.isalnum
+)
+
+add_libc_unittest(
   isalpha
   SUITE
     libc_ctype_unittests
   SRCS
     isalpha_test.cpp
   DEPENDS
     libc.src.ctype.isalpha
 )
+
+add_libc_unittest(
+  isdigit
+  SUITE
+    libc_ctype_unittests
+  SRCS
+    isdigit_test.cpp
+  DEPENDS
+    libc.src.ctype.isdigit
+)

libc/test/src/ctype/isalnum_test.cpp

+//===-- Unittests for isalnum----------------------------------------------===//
+//
+// 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/isalnum.h"
+#include "utils/UnitTest/Test.h"
+
+// Helper function that makes a call to isalnum a bit cleaner
+// for use with testing utilities, since it explicitly requires
+// a boolean value for EXPECT_TRUE and EXPECT_FALSE.
+bool call_isalnum(int c) { return __llvm_libc::isalnum(c); }
+
+TEST(IsAlNum, DefaultLocale) {
+  // Loops through all characters, verifying that numbers and letters
+  // return true and everything else returns false.
+  for (int c = 0; c < 255; ++c) {
+    if (('a' <= c && c <= 'z') || ('A' <= c && c <= 'Z') ||
+        ('0' <= c && c <= '9'))
+      EXPECT_TRUE(call_isalnum(c));
+    else
+      EXPECT_FALSE(call_isalnum(c));
+  }
+}

libc/test/src/ctype/isdigit_test.cpp

+//===-- Unittests for 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 "utils/UnitTest/Test.h"
+
+// Helper function that makes a call to isdigit a bit cleaner
+// for use with testing utilities, since it explicitly requires
+// a boolean value for EXPECT_TRUE and EXPECT_FALSE.
+bool call_isdigit(int c) { return __llvm_libc::isdigit(c); }
+
+TEST(IsDigit, DefaultLocale) {
+  // Loops through all characters, verifying that numbers return true
+  // and everything else returns false.
+  for (int ch = 0; ch < 255; ++ch) {
+    if ('0' <= ch && ch <= '9')
+      EXPECT_TRUE(call_isdigit(ch));
+    else
+      EXPECT_FALSE(call_isdigit(ch));
+  }
+}