A lot of disturbing compiler complaints (char vs. unsigned char)

[frispete]

Dear Doug,

thanks for providing and sharing this nice tool. I discovered it today, thanks to an article in the c't, a highly regarded computer magazine here in Germany. In order to spread the joy, I packaged v1.3.3 for openSUSE right away here, in such a way, that it's ready for entering the official distribution, building on the blocks from Antoine Ginies. During that course, I noticed a lot of rather disturbing compiler complaints during build:

[    5s] gcc -DHAVE_CONFIG_H -I.  -DDATADIR=\"/usr/share\" -DSYSCONFDIR=\"/etc\"   -O2 -Wall -D_FORTIFY_SOURCE=2 -fstack-protector-strong -funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection -Werror=return-type -flto=auto -g -c -o detox.o detox.c
[    5s] detox.c: In function 'main':
[    5s] detox.c:339:29: warning: pointer targets in passing argument 1 of 'parse_file' differ in signedness [-Wpointer-sign]
[    5s]   339 |      file_work = parse_file(*file_walk, main_options);
[    5s]       |                             ^~~~~~~~~~
[    5s]       |                             |
[    5s]       |                             char *
[    5s] In file included from detox.c:46:
[    5s] file.h:38:49: note: expected 'unsigned char *' but argument is of type 'char *'
[    5s]    38 | extern unsigned char *parse_file(unsigned char *filename, struct detox_options *options);
[    5s]       |                                  ~~~~~~~~~~~~~~~^~~~~~~~
[    5s] detox.c:339:16: warning: pointer targets in assignment from 'unsigned char *' to 'char *' differ in signedness [-Wpointer-sign]
[    5s]   339 |      file_work = parse_file(*file_walk, main_options);
[    5s]       |                ^
[    5s] detox.c:340:16: warning: pointer targets in passing argument 1 of 'parse_dir' differ in signedness [-Wpointer-sign]
[    5s]   340 |      parse_dir(file_work, main_options);
[    5s]       |                ^~~~~~~~~
[    5s]       |                |
[    5s]       |                char *
[    5s] In file included from detox.c:46:
[    5s] file.h:40:38: note: expected 'unsigned char *' but argument is of type 'char *'
[    5s]    40 | extern void parse_dir(unsigned char *indir, struct detox_options *options);
[    5s]       |                       ~~~~~~~~~~~~~~~^~~~~
[    5s] detox.c:344:17: warning: pointer targets in passing argument 1 of 'parse_file' differ in signedness [-Wpointer-sign]
[    5s]   344 |      parse_file(*file_walk, main_options);
[    5s]       |                 ^~~~~~~~~~
[    5s]       |                 |
[    5s]       |                 char *
[    5s] In file included from detox.c:46:
[    5s] file.h:38:49: note: expected 'unsigned char *' but argument is of type 'char *'
[    5s]    38 | extern unsigned char *parse_file(unsigned char *filename, struct detox_options *options);
[    5s]       |                                  ~~~~~~~~~~~~~~~^~~~~~~~
[    5s] detox.c:347:20: warning: pointer targets in passing argument 1 of 'parse_special' differ in signedness [-Wpointer-sign]
[    5s]   347 |      parse_special(*file_walk, main_options);
[    5s]       |                    ^~~~~~~~~~
[    5s]       |                    |
[    5s]       |                    char *
[    5s] In file included from detox.c:46:
[    5s] file.h:42:42: note: expected 'unsigned char *' but argument is of type 'char *'
[    5s]    42 | extern void parse_special(unsigned char *in, struct detox_options *options);
[    5s]       |                           ~~~~~~~~~~~~~~~^~
[    5s] detox.c:366:20: warning: pointer targets in passing argument 1 of 'parse_inline' differ in signedness [-Wpointer-sign]
[    5s]   366 |       parse_inline(*file_walk, main_options);
[    5s]       |                    ^~~~~~~~~~
[    5s]       |                    |
[    5s]       |                    char *
[    5s] In file included from detox.c:46:
[    5s] file.h:44:41: note: expected 'unsigned char *' but argument is of type 'char *'
[    5s]    44 | extern void parse_inline(unsigned char *filename, struct detox_options *options);
[    5s]       |                          ~~~~~~~~~~~~~~~^~~~~~~~

You can see the full build log by clicking on the succeeded link.

Yes, our compilers are parameterized quite squeamishly, but it often helps to discover issues early. That doesn't work so well anymore, if a project triggers that many warnings, though...

Of course, we could muzzle the compiler, but it would be nice, if you could take a look into this issue yourself. I'm sure, eliminating these signedness issues improves the overall value of this fine project even more.

[dharple]

Thanks. I've noticed similar warnings from Clang (on Travis) and CLion. I'll address this in an upcoming release.

Unfortunately, most of the standard C functions take signed chars as their inputs, and the character transliteration logic in detox needs unsigned chars to work properly.

The code works with the implicit casts; I'll probably go with explicit casts in the v1 series and introduce more breaking changes in a major release.

[dharple]

Yeah, this is big, and potentially breaking. I'll address it in v2.0.0.

[frispete]

I feel with you.

My C brain is rusty at best, but maybe a set of wrapper macros/functions for the standard c functions with the correct casts in place would be the shortest path to success.

[frispete]

Hey, Doug, you're my hero of the day!

Thank you so much. Your approach to stick with signed chars and deal with the fallout is far superior to anything else.

[dharple]

Thanks @frispete ! I looked at your suggestion of creating wrappers around the standard C library, too, but ended up going with this approach when I realized it was only one of the filters that was failing, and I could fix it easily.

I also incorporated some of the gcc flags I saw from your output, adding protections against overflows and overruns. I like those a lot.

Thank you very much for the push in the right direction!