libc: minimal: add missing ctype.h functions #99451
Conversation
zephyrbot
requested review from
aescolar,
keith-packard,
nashif and
stephanosio
cfriedt
force-pushed
the
add-missing-ctype-functions-to-minimal-libc
branch
from
32d89a3 to
2960508
Compare
cfriedt
added
bug
|
| return (int)((c == (int)' ') || (c == (int)'\t')); | ||
| } | ||
|
|
||
| static inline int isspace(int c) |
There was a problem hiding this comment.
I'd let the compiler do something cute if it likes, but express these in a direct fashion.
static inline int isspace (int c)
{
return c == ' ' || ('\t' <= c && c <= '\r');
}
Similarly for the other functions. There's no reason to obscure what these functions do; having to check that there are 5 'blank' values between '\t' and '\r' is something the compiler should do, not us.
There was a problem hiding this comment.
I'll prepend a commit for that, since it's technically not related to the bugfix.
There was a problem hiding this comment.
I could also prepend a commit that would eliminate linter issues as well (and fix up the commit I added).
keith-packard
left a comment
keith-packard
left a comment
There was a problem hiding this comment.
Please express these directly rather than using arithmetic.
The "check warns" workflow in CI warned over the use of `unsigned` as a shorthand for `unsigned int` in several locations in `lib/libc/minimal/include/ctype.h`. ``` UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned' File:lib/libc/minimal/include/ctype.h ``` Adjust `unsigned` to `unsigned int` to avoid linter warnings. Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Based on review feedback, it was suggested to express ctype character checks in a more direct fashion, rather than using arithmetic, and allow the compiler to optimize as it sees fit. https://github.com/zephyrproject-rtos/zephyr/pull/99451#\ discussion_r2530339430 Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
Add the functions below to the minimal libc ctype.h since they are missing, and are required as of C89 (C99 for `isblank()`) * `isblank()` * `islower()` * `ispunct()` Signed-off-by: Chris Friedt <cfriedt@tenstorrent.com>
cfriedt
force-pushed
the
add-missing-ctype-functions-to-minimal-libc
branch
from
2960508 to
c99031e
Compare
|
keith-packard
left a comment
keith-packard
left a comment
There was a problem hiding this comment.
Please remove all of the unnecessary casts; C is at least semi-reasonable for these cases.
| static inline int isupper(int a) | ||
| { | ||
| return (int)(((unsigned)(a)-(unsigned)'A') < 26U); | ||
| return (int)(((int)'A' <= a) && (a <= (int)'Z')); |
There was a problem hiding this comment.
You don't need any casts in this expression -- character constants are of type int, the relational operators result is of type int and the logical AND operator result is of type int. This should be just return 'A' <= a && a <= 'Z';
Feel free to add parentheses if you like, they aren't required as the precedence works correctly for this case.
| } | ||
|
|
||
| /* force to lowercase */ | ||
| a |= 32U; |
There was a problem hiding this comment.
tolower uses addition instead of bitwise OR; I'd be consistent here.
4 participants
Add the functions below to the minimal libc
ctype.hsince they are missing, and are required as of C89 (C99 forisblank())isblank()islower()ispunct()Fixes #99452