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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/include/ctype.h.def:1
+//===-- C standard library header ctype.h --------------------------------===//
+//
----------------
I think we have a `ctype.h` file already present. It was added when I was setting up examples. But, `ctype.h.def ` as you have added is the right approach. Can you remove `ctype.h` in a separate patch and add appropriate targets for `ctype.h.def` in this patch? Also, you do need to add an entry to `linux/api.td` isn't it?


================
Comment at: libc/src/ctype/isalpha.h:14
+
+// TODO: Currently restricted to default locale.
+// These should be extended using locale information.
----------------
Can you move this TODO to the `.cpp` file?


================
Comment at: libc/test/src/ctype/isalpha_test.cpp:11
+#include "utils/UnitTest/Test.h"
+
+// Helper function that makes a call to isalpha a bit cleaner
----------------
Wouldn't it be simple if we have just one test like this:

```
TEST(IsAlpha, DefaultLocale) {
  for (int i = 0; i < 255; ++i) {
    if (('a' <= i && i <= 'z') || ('A' <= i && i <= 'Z')
      ASSERT_TRUE(...);
    else
      ASSERT_FALSE(...);
  }
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84575