Improved c89 compatibility

[dirkson]

Heyo!

Currently your code fails to compile when c_std=gnu89. However, the only C99 feature that you appear to use is the ability to declare variables inside for loops. This PR moves the declarations outside the for loops, and compiles when c_std=gnu89

Cheers!

[ludocode]

Hi there! Thanks for the PR. I'm not sure it's a good idea to merge it though.

As it turns out there are quite a few C99 features used in MPack, and some of them would be difficult to get away from. There are // line comments, variadic macros, trailing commas in enumerator lists, fixed width integer types and their format macros, the new bool type, const, restrict, inline, and probably more stuff I can't think of right now.

The biggest one though is mixed declarations and code. This would be quite a large undertaking to change, and I don't think it would be an improvement. I know some would disagree with me (Linus for example) but I really think mixed declarations helps readability and safety. It's quite powerful to be able to declare a variable at the point in a block where it's needed rather than at the top. This combined with the for loop declaration makes it so that nearly all variables can be declared with an initializer (and without a spurious block) even if earlier computations or error checks are needed. This makes a declaration without an initializer stand out, so that you know to follow the control flow to make sure it's set before being read (and no, warnings still can't catch all cases of this, such as when a pointer to a local variable is passed to a function that reads from it.)

There is also the problem of being able to keep this unit tested. The MPack unit test suite is built with at least -Wall -Wextra -Wpedantic -Werror in all configurations. With these flags and -std=gnu89, the compiler warns against many of the above features being used, so the unit test suite spits out hundreds of errors.

Of course the for loop thing seems to be the only one that can't be worked around with a compiler warning or pragma. We can use e.g. -Wno-declaration-after-statement -Wno-variadic-macros -Wno-pedantic and silence pretty much all of the above warnings in -std=gnu89 mode, but not the for loop thing. I know you're only asking for gnu89 support where all the rest of this stuff is allowed, and this would get you there. Unfortunately this adds a ton of declarations without initializers (there are way more declarations in for loops in the MPack unit test suite than in the MPack source.) I'm not sure I'm comfortable with that.

Given the number of other C99 features MPack uses, I don't think it's worthwhile trying to support it. I'm open to being convinced though. The kernel folks use all sorts of C99 features and GNU extensions but they still explicitly use -std=gnu89 and they still aren't using mixed declarations or declarations in for loops. I guess what I'm wondering right now is, other than the kernel, are there many projects that are intentionally sticking with not C89, but gnu89 with all its extensions? Is it worth supporting gnu89 in this project when we are very far from being able to support C89 proper?

[dirkson]

I like your response - it's incredibly well thought out.

A quick 'me too' to start. I agree with you about mixed declarations - Clearly a variable with a restricted scope is easier to read and harder to screw up than a variable with a less restricted scope.

So there's a couple things going on here. First off, I was definitely suggesting gnu89 compatibility, rather than raw c89/c90/ansi compatibility. It DOES have things like // comments, inline (although it means something else), etc. etc. You do mention that at one point, but also talk about plain C89 support a lot, which I absolutely agree is not reasonable for a project to support in this day and age.

Secondly, I was using a different build system on your code, which automatically stripped out the -Wpedantic flag and didn't run the unit tests, which is why I didn't notice the incompatibilities there. Oops!

But wait a second, you're using 'restrict', aren't you? That was definitely added in C99. So why isn't gcc erroring?

It turns out that gnu89 allows in (some) c99 features, like you point out. When was that decided? At least one, inline, differs greatly in implementation from its c99 sibling - Where is the canonical description of how it behaves, again? Without a standard to point to, answering questions like this becomes rather difficult.

I generally try to use the least complicated thing that works, which was my rationale for sticking with gnu89. But this experience has made me realize that gnu89 ISN'T less complicated than C99. It adds in most of C99's features in a way that isn't compliant to any standard. I probably should have noticed that far, far sooner, but it's easy to miss when nothing is throwing an error at you and you've got a Goal to reach.

Long story short, I've been convinced that targeting C99 as the minimum standard is a better idea than targeting gnu89. I've also become impressed with the pedantic flag, and I think I'll start including that in my own software. I agree that we should ignore my PR, so I'll close this.

Thanks for chatting this out with me.

Cheers!

[ludocode]

I'm discussing supporting gnu89 after all in #81. This PR can't be re-opened because your fork has been deleted, but of course it can be merged manually. @dirkson Are you OK with me merging this?

[dirkson]

Oh, go for it! Let me know if there's anything I can do to make that easier.

I'll keep an eye on where you end up with this all this.

[ludocode]

This is now merged, along with fixes to the for loops in the unit tests. I've added a gnu89 unit test configuration to ensure that this stays fixed going forward, and I will be adding more fixes to allow MPack to be built in a Linux kernel module. This will be included in the upcoming MPack 1.1 release. Thanks again for the PR.