[libc-commits] [PATCH] D84575: [libc] Add scaffolding for ctype and implementation of isalpha

[Alex Brachet via Phabricator via libc-commits]

abrachet added inline comments.


================
Comment at: libc/config/linux/aarch64/entrypoints.txt:5
+    
+    # ctype.h entrypoints
+    libc.src.ctype.isalpha
----------------
Nit: Maybe these should go above errno.h entrypoints for alphabetical order


================
Comment at: libc/config/linux/api.td:408
+def CTypeAPI : PublicAPI<"ctype.h"> {
+  let TypeDeclarations = [];
+  
----------------
Presumably not necessary? You don't have empty Macros so presumably it is fine to leave this out for now.


================
Comment at: libc/src/ctype/isalpha.cpp:16
+int LLVM_LIBC_ENTRYPOINT(isalpha)(int c) {
+  const unsigned int ch = c;
+  return (ch | 32) - 'a' < 26;
----------------
Just use `unsigned`


================
Comment at: libc/test/src/ctype/isalpha_test.cpp:14
+// for use with testing utilities.
+bool call_isalpha(int c) { return static_cast<bool>(__llvm_libc::isalpha(c)); }
+
----------------
You don't need the cast.


================
Comment at: libc/test/src/ctype/isalpha_test.cpp:26-34
+TEST(IsAlpha, PostFixOperatorOnBoundaryCharacterShouldStillWork) {
+  int character = 'Z';
+  // This should return true, even though it has side effects.
+  // This may fail in the case of conditional logical operators
+  // being used in the 'isalpha' implementation.
+  EXPECT_TRUE(call_isalpha(character++));
+  // Verify the next character is indeed not true.
----------------
I don't understand the comment, there is no implementation where `character` would pass but not `character++`. You're testing the compiler here, not `isalpha`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84575/new/

https://reviews.llvm.org/D84575