[libc-commits] [PATCH] D84893: [libc] Implements isdigit and isalnum.

[Siva Chandra via Phabricator via libc-commits]

sivachandra accepted this revision.
sivachandra added a comment.
This revision is now accepted and ready to land.

In D84893#2184401 <https://reviews.llvm.org/D84893#2184401>, @cgyurgyik wrote:

> In D84893#2183796 <https://reviews.llvm.org/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.



================
Comment at: libc/src/ctype/ctype_utils.h:20
+
+static inline int inlined_isdigit(int c) {
+  const unsigned ch = c;
----------------
Instead of the `inlined_` prefix, may be put them in a nested namespace `internal`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84893