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

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik marked 2 inline comments as done.
cgyurgyik added inline comments.


================
Comment at: libc/include/ctype.h.def:1
+//===-- C standard library header ctype.h --------------------------------===//
+//
----------------
sivachandra wrote:
> 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?
I will remove ctype.h immediately following the submission of this revision.


================
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
----------------
sivachandra wrote:
> 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(...);
>   }
> }
> ```
> 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(...);
>   }
> }
> ```

Yep that works as well. It definitely breaks the idea of unit testing different behaviors, but isalpha is probably simple enough that this won't matter a whole lot. I mostly wasn't sure if 255 calls to `is*` would get hefty after a few more of these are implemented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84575