HiRedis has certain issues when called by another project that is on C89/C90

[dagostinelli]

I'd like to re-open discussion about this:
#31

TLDR: Let's make hiredis header files C89/C90 compatible. Not the whole project.

I have a hard requirement to go C89/C90 in a project where I'd like to use hiredis. When I try to use the hiredis header files, a number of problems emerge that are hard to work around. It would be better if hiredis's header files were C89/C90 compatible.

This can be accomplished in a few different ways. I'd like to open discussion on this.

Approach 1: Write a hello,world test app (not in the examples folder -- because this wouldn't be an example) that uses hiredis with compiler flags set to -std=c90 -pedantic to start (potentially other flags to increase the strictness of the header files) If this .c file creates compiler warnings, they can be caught by developers during check-in time.

Approach 2 Append certain compiler flags to the Makefile. This would have a broader impact on the project.
Where it says:
WARNINGS=-Wall -W -Wstrict-prototypes -Wwrite-strings
Add some compiler flags such as:
-pedantic-errors -Werror -Wextra -Werror -Wmissing-prototypes -Wundef -Wpointer-arith -Wcast-qual -Wcast-align -Wfloat-equal -Wconversion -Wno-missing-braces -Wold-style-definition -Wdeclaration-after-statement
(The appropriate list of flags is debatable)

Approach 3 Make the examples C89/C90 compliant by adding compiler flags just on the examples.

[dagostinelli]

I tried Approach 3, here's the result:

cc -o examples/hiredis-example -std=c90 -pedantic -Wextra -Werror -Wmissing-prototypes -O3 -fPIC  -Wall -W -Wstrict-prototypes -Wwrite-strings -g -ggdb    -I. examples/example.c libhiredis.a
In file included from ./hiredis.h:36:0,
                 from examples/example.c:5:
./read.h:75:55: error: ISO C90 does not support ‘long long’ [-Werror=long-long]
     void *(*createInteger)(const redisReadTask*, long long);
                                                       ^~~~
In file included from ./hiredis.h:40:0,
                 from examples/example.c:5:
./sds.h:48:10: error: ISO C90 does not support flexible array members [-Werror=pedantic]
     char buf[];
          ^~~
./sds.h:54:10: error: ISO C90 does not support flexible array members [-Werror=pedantic]
     char buf[];
          ^~~
./sds.h:60:10: error: ISO C90 does not support flexible array members [-Werror=pedantic]
     char buf[];
          ^~~
./sds.h:66:10: error: ISO C90 does not support flexible array members [-Werror=pedantic]
     char buf[];
          ^~~
./sds.h:72:10: error: ISO C90 does not support flexible array members [-Werror=pedantic]
     char buf[];
          ^~~
./sds.h:86:8: error: unknown type name ‘inline’
 static inline size_t sdslen(const sds s) {
        ^~~~~~
./sds.h:86:22: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘sdslen’
 static inline size_t sdslen(const sds s) {
                      ^~~~~~
./sds.h:103:8: error: unknown type name ‘inline’
 static inline size_t sdsavail(const sds s) {
        ^~~~~~
./sds.h:103:22: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘sdsavail’
 static inline size_t sdsavail(const sds s) {
                      ^~~~~~~~
./sds.h:129:15: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
 static inline void sdssetlen(sds s, size_t newlen) {
               ^~~~
./sds.h:153:15: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
 static inline void sdsinclen(sds s, size_t inc) {
               ^~~~
./sds.h:179:8: error: unknown type name ‘inline’
 static inline size_t sdsalloc(const sds s) {
        ^~~~~~
./sds.h:179:22: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘sdsalloc’
 static inline size_t sdsalloc(const sds s) {
                      ^~~~~~~~
./sds.h:196:15: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘void’
 static inline void sdssetalloc(sds s, size_t newlen) {
               ^~~~
./sds.h:247:26: error: ISO C90 does not support ‘long long’ [-Werror=long-long]
 sds sdsfromlonglong(long long value);
                          ^~~~
In file included from examples/example.c:5:0:
./hiredis.h:114:10: error: ISO C90 does not support ‘long long’ [-Werror=long-long]
     long long integer; /* The integer when type is REDIS_REPLY_INTEGER */
          ^~~~
examples/example.c: In function ‘main’:
examples/example.c:14:45: error: C++ style comments are not allowed in ISO C90
     struct timeval timeout = { 1, 500000 }; // 1.5 seconds
                                             ^
examples/example.c:14:45: error: (this will be reported only once per input file)
examples/example.c:47:12: error: ISO C90 does not support the ‘ll’ gnu_printf length modifier [-Werror=format=]
     printf("INCR counter: %lld\n", reply->integer);
            ^~~~~~~~~~~~~~~~~~~~~~
examples/example.c:51:12: error: ISO C90 does not support the ‘ll’ gnu_printf length modifier [-Werror=format=]
     printf("INCR counter: %lld\n", reply->integer);
            ^~~~~~~~~~~~~~~~~~~~~~
examples/example.c:60:9: error: implicit declaration of function ‘snprintf’ [-Werror=implicit-function-declaration]
         snprintf(buf,64,"%u",j);
         ^~~~~~~~

[michael-grunder]

Hey,

First off I am not all that familiar with C90, but from the above output, it seems non-trivial to make hiredis compliant.

For instance, these errors with the sds string:

./sds.h:48:10: error: ISO C90 does not support flexible array members [-Werror=pedantic]
     char buf[];

The string library is defining a zero length member at the end of the struct. The only way to do this in C90 (afaik) is to define it like char buf[1] then do +1 -1 math on allocation/reallocation/etc.

The problem though, is that the sds string library is pulled from upstream from time to time, so you would need to get support for the changes there as well (https://github.com/antirez/sds).

The project you're working on is limited to C90 full stop?

[dagostinelli]

The project you're working on is limited to C90 full stop?

Basically, yes. The work around would have to be to wrap hiredis in a C90 compliant wrapper and get on with the rest of the project. That's not ideal.

I'll make the case with the SDS lib as well.

I was just now able to make a number of changes to hiredis in the header files and get at least the libEvent example to compile. I doubt my changes were 100% correct as this was a first shot at it. My sds changes seem off somehow and I haven't tested it yet. But it does compile now. Is there anyway to not have sds.h in the hiredis headers?

For comparison, libUV is not C90 compatible, but libEvent is. So at least getting hiredis' headers to C90 by using just the libEvent sample as the path forward seems possible right now. But there's that SDS issue.

I'll play around some more.

[dagostinelli]

(this is a long shot -- just wondering)

The impact to hiredis by removing sds.h from the headers would be the elimination of two APIs:

int redisFormatSdsCommandArgv(sds *target, int argc, const char ** argv, const size_t *argvlen);
void redisFreeSdsCommand(sds cmd);

Is that remotely in scope?

[dagostinelli]

Another idea instead of removing them, maybe they can be moved to an aux header, sort of like how the adapters work?

[dagostinelli]

Here's a pot shot at this. I know the SDS stuff is wrong. How about the rest though?

https://github.com/dagostinelli/hiredis/commit/155cf5b56015874ccf72089e692a0296042853c0

[michael-grunder]

Coincidentally, I actually wrote those two API methods, so you can blame me for a lot of this pain. 😄 They were added because they are a substantial speed improvement.

Instead of moving them to an adapter include file, perhaps there is a portable way to check the C standard and just wrap the definitions in #if blocks (and I suppose the sds.h include as well).

That certainly makes things simpler. Then it looks mostly to be type issues (long long, etc).

@badboy Thoughts on this?

[dagostinelli]

I've made an attempt at it.

• detects C90 (might be gcc only though -- need to test this on other compilers)
• replaces long long with int64_t
• enforces C90 only on the libevent sample (which is already has C90 compatible header files) and thus enforces it on the core hiredis headers (hiredis.h, read.h)

Apply patch with git apply

c90-attempt-1.patch.zip

[badboy]

Please just submit the patch as a PR and I can take a look at it.

[dagostinelli]

#495

[dagostinelli]

There is still a problem with the patch I just submitted -- when I go to use this in my C90 project, it complains about the static functions in libevent.h

hiredis/adapters/libevent.h:88:12: error: ‘redisLibeventAttach’ defined but not used [-Werror=unused-function]
 static int redisLibeventAttach(redisAsyncContext *ac, struct event_base *base) {

I'm thinking about a solution as I write this...

[dagostinelli]

Nevermind... that undefined thing was just my code. False alarm. I think the patch is complete as far as compiling on gcc.

[badboy]

I thought about this. First, thanks for your work on this.

However, I dediced not to maintain C90 compatibility. Even though the current changes are minimal, I am less than thrilled to support a superseded standard at this point.

[dagostinelli]

Then you have undermined the library's wide appeal. A single library doesn't get to decide for others which standard they want to accept.

[badboy]

That is true. But I'm not responsible for other project's decisions and I am doing this on my free time and thus it's up to me to decide what to maintain and support.
hiredis is free and open source and those who need C90-compatibility need to maintain it themselves.

[dagostinelli]

My last thought on this, then I'll let it go. I really appreciate your work too!

those who need C90-compatibility need to maintain it themselves.

I think this might be how forks end up all over. My PR was minimal and so it avoids a whole fork. I'm just asking for compliance in the headers, not the whole thing.

• The standard was purposefully meant to be backwards compatible. If you can keep the headers C90, you reach a wider audience.

• This on GitHub where other folks can literally help you maintain this. You aren't entirely alone. Let us help you. Really -- that's what we are doing. I've even put a couple of compiler rules into your build script that will help enforce this. Your level of effort on-going isn't huge.

• This library is, to be blunt, a hugely important library.

Summary: Let the headers be C90 compatible. Wider audience == Good; Less Forks == Good; Level of Effort == Extremely-Low; The samples are even slightly better coded now (int64_t and static) Maybe ask for donations.