Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix many calls to ctype functions #3734

Merged
merged 1 commit into from
May 26, 2023
Merged

Conversation

b4n
Copy link
Member

@b4n b4n commented May 24, 2023

All ctype functions like isspace(), tolower() and alike expect a character as an unsigned char or EOF, anything else has undefined behavior.

This commit should fix all calls to the various ctype functions either:

  • adding unsigned char cast where appropriate;
  • removing useless int casts that are more confusing than useful.

Some instances required slightly more involved changes though, but the fix itself is the same.


That one was long, boring and a bit tricky. I think I didn't miss any calls (used some grepping and went through it all), but I could have let one slip somehow I guess… reviewing will likely be just as tricky, sorry.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage: 85.13% and project coverage change: -0.01 ⚠️

Comparison is base (979433d) 83.00% compared to head (a76c937) 82.99%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3734      +/-   ##
==========================================
- Coverage   83.00%   82.99%   -0.01%     
==========================================
  Files         226      226              
  Lines       55012    55037      +25     
==========================================
+ Hits        45661    45678      +17     
- Misses       9351     9359       +8     
Impacted Files Coverage Δ
main/kind.c 98.09% <ø> (ø)
main/options.c 83.83% <ø> (ø)
parsers/make.c 100.00% <ø> (ø)
parsers/perl6.c 96.55% <ø> (ø)
parsers/verilog.c 98.34% <ø> (ø)
main/routines.c 81.41% <28.57%> (ø)
main/args.c 51.96% <33.33%> (ø)
parsers/beta.c 39.61% <35.29%> (-0.81%) ⬇️
parsers/asp.c 46.93% <38.23%> (ø)
parsers/lisp.c 89.83% <60.00%> (ø)
... and 40 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@masatake
Copy link
Member

You may use sed or grep to find the lines to change in this pull request.
Could you those command lines to the commit log?

@b4n
Copy link
Member Author

b4n commented May 25, 2023

You may use sed or grep to find the lines to change in this pull request. Could you those command lines to the commit log?

Done. I actually used Geany's Search in files feature so I could easily jump between matches with a lot less risk of missing one in that loong list, but it's just an interface to grep. I couldn't do any sed though as I had to review each call manually for the type of the argument before I could decide how it had to be altered.

@masatake
Copy link
Member

Thank you for updating the commit log.

I have one complicated request.

About libreadtags/readtags.c, could you remove the change from this pull request?
libreadtags/readtags.c is maintained separately at https://github.com/universal-ctags/libreadtags

So could you make another pull request for the repository about
libreadtags/readtags.c.

After merging this pull request (without changes about libreadtags/readtags.c) and
the pull request that you will kindly open at https://github.com/universal-ctags/libreadtags,
I will request this repository (https://github.com/universal-ctags/ctags) to pull in your change at https://github.com/universal-ctags/libreadtags to this repository.
pull-libreadtags.sh is the script for pulling in.

All ctype functions like `isspace()`, `tolower()` and alike expect a
character as an `unsigned char` or EOF, anything else has undefined
behavior.

This commit should fix all calls to the various ctype functions either:

* adding `unsigned char` cast where appropriate;
* removing useless `int` casts that are more confusing than useful.

Some instances required slightly more involved changes though, but the
fix itself is the same.

To find the calls to check, I used an equivalent to the following:

```
git grep -P 'is(alnum|alpha|cntrl|x?digit|graph|lower|print|punct|'\
'space|upper|ascii|blank)|to(low|up)er'
```

...and then proceeded to manually check each one of them to see if the
call, or call chain, was using appropriate values and types.
@b4n
Copy link
Member Author

b4n commented May 25, 2023

I have one complicated request.

About libreadtags/readtags.c, could you remove the change from this pull request? libreadtags/readtags.c is maintained separately at https://github.com/universal-ctags/libreadtags

So could you make another pull request for the repository about libreadtags/readtags.c.

Sure, no problem, and it wasn't really complicated at all :) It's done, and the companion PR at libreadtags is universal-ctags/libreadtags#49

@masatake
Copy link
Member

masatake commented May 25, 2023

Find. Fine. Please, merge this!

@b4n b4n merged commit 23bacc1 into universal-ctags:master May 26, 2023
@masatake
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants