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

Does not compile cleanly #61

Closed
al20878 opened this issue Oct 21, 2022 · 6 comments
Closed

Does not compile cleanly #61

al20878 opened this issue Oct 21, 2022 · 6 comments

Comments

@al20878
Copy link
Contributor

al20878 commented Oct 21, 2022

I tried to build this library with GCC 11 on Cygwin just now, and it does not compile cleanly:

In file included from editline.c:23:
editline.c: In function 'do_case':
editline.c:497:29: warning: array subscript has type 'char' [-Wchar-subscripts]
  497 |                 if (islower(*p))
      |                             ^~
editline.c:499:32: warning: array subscript has type 'char' [-Wchar-subscripts]
  499 |             } else if (isupper(*p)) {
      |                                ^~
editline.c: In function 'argify':
editline.c:1878:28: warning: array subscript has type 'char' [-Wchar-subscripts]
 1878 |     for (c = line; isspace(*c); c++)
      |                            ^~
editline.c:1885:22: warning: array subscript has type 'char' [-Wchar-subscripts]
 1885 |         if (!isspace(*c)) {
      |                      ^~
  CC       libeditline_la-complete.lo
In file included from complete.c:23:
complete.c: In function 'rl_find_token':
complete.c:281:35: warning: array subscript has type 'char' [-Wchar-subscripts]
  281 |         if (isspace(rl_line_buffer[pos])) {
      |                     ~~~~~~~~~~~~~~^~~~~
complete.c:289:47: warning: array subscript has type 'char' [-Wchar-subscripts]
  289 |     while (pos >= 0 && !isspace(rl_line_buffer[pos])) {
      |                                 ~~~~~~~~~~~~~~^~~~~

A "char" subscript can actually be a big deal for characters with the 8th bit set (will get converted to a negative "int"), so the warnings do not look great.
Any chance you can fix these please (e.g. by adding explicit (unsigned char) casts where necessary)?
Thanks.

P.S. BTW the "Build & Install" instructions in README.md call for running configure but it is not provided in the source tree if cloned directly from github, and it looks like autogen.sh needs to be executed first in order to actually build configure -- but that step is missing entirely from the instructions.

@rofl0r
Copy link
Contributor

rofl0r commented Oct 21, 2022

related: https://drewdevault.com/2020/09/25/A-story-of-two-libcs.html (explains why the first such warnings can happen at all)

@troglobit
Copy link
Owner

Hi! Of course I can fix this, but my timeline may not be in sync with yours, so if you want a quick turnaround on this you can submit a pull request yourself to speed things up.

Re autogen: yeah, I've missed documenting step 0. call automake.sh if you build from git. Again, pull requests are welcome ;-)

I maintain a lot of packages and can't make any promises when I'll circle back to editline. But when I do, I always make sure to go through the list of active issues.

Thanks for your report.

@troglobit
Copy link
Owner

@al20878 any news? I'd love to see a PR from you

Same goes for you @rofl0r, you've been lurking around my repos far too long now. Step it up!

@rofl0r
Copy link
Contributor

rofl0r commented Oct 22, 2022

"clean compile" is overrated and often leads to "fixes" that are actually bugs. in this case here the warnings stem from cygwin/glibc's semi-bogus implementation of ctype.h funcs...

@al20878
Copy link
Contributor Author

al20878 commented Oct 22, 2022

I'd love to see a PR from you

@troglobit I'll post when I have a minute... Weekend is a busy time for me :-)

semi-bogus implementation of ctype.h funcs

@rofl0r Actually, glibc implementation of the character classification macros does allow negative indices for the very reason that folks feed the char-type values in those often with the 8th bit set... And having warnings like these is (IMO) better than getting SIGSEGV in those implementations that silently compile yet can't deal with values outside the [-1..255] range in run-time.

@troglobit
Copy link
Owner

@rofl0r OMG what are you on about? Do you really think I cannot make out the difference between a "fix" and an actual code improvement? Let me be the judge of pull requests in projects I'm the owner of. After all, I built this little project up from its humble beginnings in the Minix project almost 15 years ago ... I'm not a fool. The code in question here is actually very interesting wrt. where we want to take the project going forward (unicode, deeply embedded, ...), so all eyes on stuff like the nasty business ctype.h brings is very welcome!

I asked you to step it up since you've commented in the past, and even liked comments from other people here who I've blocked (I see it all), but you've never really contributed anything useful yourself. I don't know who you are since you insist on remaining anonymous, so I cannot give you the benefit of a doubt being a friend or an acquaintance, which I guess would be considered favoritism anyway. So my message remains; step it up (carefully) or step off, thank you.

al20878 added a commit to al20878/editline that referenced this issue Oct 23, 2022
al20878 added a commit to al20878/editline that referenced this issue Oct 23, 2022
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

No branches or pull requests

3 participants