This is an archive of the discontinued LLVM Phabricator instance.

[libc] Implements isdigit and isalnum.
ClosedPublic

Authored by cgyurgyik on Jul 29 2020, 3:18 PM.

Diff Detail

Event Timeline

cgyurgyik created this revision.Jul 29 2020, 3:18 PM
cgyurgyik requested review of this revision.Jul 29 2020, 3:18 PM
cgyurgyik added a reviewer: sivachandra.

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?

cgyurgyik added a comment.EditedJul 30 2020, 4:52 AM

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.
  1. 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.
cgyurgyik updated this revision to Diff 281906.Jul 30 2020, 6:07 AM

To demonstrate my thoughts, adds inlined versions of functions isalpha, isdigit in a utility header.

cgyurgyik updated this revision to Diff 281934.Jul 30 2020, 8:15 AM
sivachandra accepted this revision.Jul 30 2020, 9:21 AM

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.

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

libc/src/ctype/ctype_utils.h
21

Instead of the inlined_ prefix, may be put them in a nested namespace internal?

This revision is now accepted and ready to land.Jul 30 2020, 9:21 AM

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.

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

cgyurgyik updated this revision to Diff 281960.Jul 30 2020, 9:34 AM
cgyurgyik marked an inline comment as done.
This revision was landed with ongoing or failed builds.Jul 30 2020, 9:39 AM
This revision was automatically updated to reflect the committed changes.