Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
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.
- 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'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.
To demonstrate my thoughts, adds inlined versions of functions isalpha, isdigit in a utility header.
I am not sure I understand what you mean by defining twice.
- 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 | ||
---|---|---|
20 | Instead of the inlined_ prefix, may be put them in a nested namespace internal? |
I meant that the LLVM_LIBC_ENTRYPOINT(isdigit) should just call internal::isdigit. I think we're on the same page.
Instead of the inlined_ prefix, may be put them in a nested namespace internal?