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

[Chris Gyurgyik via Phabricator via libc-commits]

cgyurgyik added a comment.

In D84893#2185074 <https://reviews.llvm.org/D84893#2185074>, @sivachandra wrote:

> 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.

I meant that the `LLVM_LIBC_ENTRYPOINT(isdigit)` should just call `internal::isdigit`. I think we're on the same page.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84893