Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
libc/config/linux/aarch64/entrypoints.txt | ||
---|---|---|
8 | Nit: Maybe these should go above errno.h entrypoints for alphabetical order | |
libc/config/linux/api.td | ||
416 | Presumably not necessary? You don't have empty Macros so presumably it is fine to leave this out for now. | |
libc/src/ctype/isalpha.cpp | ||
17 | Just use unsigned | |
libc/test/src/ctype/isalpha_test.cpp | ||
15 | You don't need the cast. | |
27–35 | 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 |
libc/test/src/ctype/isalpha_test.cpp | ||
---|---|---|
27–35 | This was written after reading the wikipedia page: https://en.wikipedia.org/wiki/C_character_classification#Implementation I am happy to remove it if you think this is a compiler test instead. |
libc/include/ctype.h.def | ||
---|---|---|
2 | 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? | |
libc/src/ctype/isalpha.h | ||
15 | Can you move this TODO to the .cpp file? | |
libc/test/src/ctype/isalpha_test.cpp | ||
12 | 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(...); } } |
Update unit tests to a single test that goes through all characters 0-255.
Re-add linux/api.td.
Address other comments
libc/include/ctype.h.def | ||
---|---|---|
2 | I will remove ctype.h immediately following the submission of this revision. | |
libc/test/src/ctype/isalpha_test.cpp | ||
12 |
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. |
Nit: Maybe these should go above errno.h entrypoints for alphabetical order