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

Drop C89 support? #6691

Closed
slouken opened this issue Nov 28, 2022 · 37 comments
Closed

Drop C89 support? #6691

slouken opened this issue Nov 28, 2022 · 37 comments
Milestone

Comments

@slouken
Copy link
Collaborator

slouken commented Nov 28, 2022

What does @libsdl-org/a-team think about dropping C89 support?

Off the top of my head, we can drop Sint*/Uint* in favor of types in stdint.h, we can potentially drop SDL_bool in favor of stdbool.h, we can use // comments, etc.

I think all the compilers for platforms that we support have a C99 or newer mode available.

Thoughts?

@slouken slouken added this to the 3.2.0 milestone Nov 28, 2022
@slouken
Copy link
Collaborator Author

slouken commented Nov 28, 2022

One of the things I want to be aware of is the impact of code changes in SDL 3 affecting merges between SDL 2 and SDL 3. We're going to be supporting both and fixes in one are likely to be wanted in the other, so I want to be careful that we're not making that harder by lots of sweeping code changes.

@sezero
Copy link
Contributor

sezero commented Nov 28, 2022

What shall we really gain from all this? Namely:

Off the top of my head, we can drop Sint*/Uint* in favor of types in stdint.h

Our types are already sized, if that's the concern, but I'm not
strongly objecting to it or something.

we can potentially drop SDL_bool in favor of stdbool.h

And change size of SDL_bool from 4 (sizeof(int)) to 1? What shall
that gain us?

we can use // comments, etc.

I'd say blah, but suggest that you keep out of public headers: you
will find users that build with -pedantic or similar and come back
to you whining.

@mupfdev
Copy link
Contributor

mupfdev commented Nov 28, 2022

Hello,
I have always seen C89 support as SDL's greatest strength, especially since the practical difference to C99 is mostly negligible. One quickly gets used to declare variables at the beginning of a scope without any noteworthy disadvantage. In my personal projects, I exclusively use the standard integer types from SDL. I also use SDL's Standard Library Functionality wherever I can. I also write all my code in C89. Why? Because I know for a fact that if it works on my N-Gage, I don't have to worry about whether it works on another platform or not. It is my gauge when it comes to code portability.

Speaking of the N-Gage: A parting with C89 would also be the end of the Nokia N-Gage port. The Symbian S60v1 SDK neither provides stdint.h nor does the compiler support C99. I would very much regret this, as the homebrew scene for this platform is still in its infancy.

An update to a more modern compiler would be a possibility, but it would also be anything but trivial. Support for ARM PE executables has been dropped after GCC version 4.6.4 in 2013. However, support was deactivated much earlier by default.

But out of sincere interest: what would be the notable benefits of parting with C89?

My two cents.
Michael

@ccawley2011
Copy link
Contributor

Off the top of my head, we can drop Sint*/Uint* in favor of types in stdint.h

Technically, SDL already depends on stdint.h, since even in SDL1 and SDL2, types like Uint32 are only ever defined as uint32_t, and platforms that don't support C99 types provide definitions of those in the platfom-specific config files. Since SDL3 moved the config files out of the public API, this does mean that SDL3 builds are currently broken for N-Gage and MSVC <= 2008. The previous approach also has a potential issue where the stdint.h replacements might clash if used with another library that also provides its own replacements.

Personally, I think the best approach might be to move the replacements into SDL_stdinc.h (or a new header), change them to define Uint32 directly, and only use replacements on platforms that are known to lack stdint.h without attempting to guess any generic fallbacks. We might still need to have internal uint32_t typedefs for e.g. the Vulkan headers, but it should solve the problem with the public headers. What do you think?

(Also, it would be really nice if SDL had uint_fast16_t or an equivalent for internal use as well as the fixed size types).

@icculus
Copy link
Collaborator

icculus commented Nov 29, 2022

(Also, it would be really nice if SDL had uint_fast16_t or an equivalent for internal use as well as the fixed size types).

Fwiw, I had this listed on the old trello wishlist where I was making notes, but in recent years I'm wondering how much this would help optimization vs cause really hard to debug problems.

@icculus
Copy link
Collaborator

icculus commented Nov 29, 2022

So when I spoke to Sam about this earlier today, I thought we were talking about dumping all the configure details and moving to stdint.h to give us access to reliable types that we can use for setting up our own.

So our headers end up looking like:

#include <stdint.h>
typedef uint32_t Uint32;
// etc

(and maybe we end up with an #ifdef for old visual studios.)

But if we're talking about moving the whole codebase to stdint types...that's a hard no from me.

On a basic level: literally nothing will ever merge cleanly back into SDL2 if we make a change like that.

But also I personally love using Uint32 and have two decades of muscle memory for it, so if it's just aesthetics and personal taste, I really like the existing types.

@icculus
Copy link
Collaborator

icculus commented Nov 29, 2022

With that out of the way: there are three things I really want in C99 and miss like a lost tooth every time I write SDL code:

  • Declaring variables anywhere: they can be declared closer to where they are actually used, you can declare const vars in the middle of the function, where they can't be const if they have to be at the top of the statement block, they go out of scope more quickly. Once I had that in C, it was such a quality of life improvement that it's hard to not crave it.
  • Single-line comments. They aren't life and death, but I think they improve code quality in many instances.
  • I would deeply enjoy not having to police these two things in pull requests from humans that don't know to avoid them, and not have to fix the buildbot, where we currently intentionally treat it as an error to declare a variable mid-block.

The cons are:

  • Sticking to C89 is legitimately defensive coding, but it has paid off in the past: targets that needed gcc2, weird embedded things, old Visual Studio, etc. But my suspicion is that most modern exotic platforms are now targeting clang or a more-modern GCC, and don't have problems here. That leaves exotic ancient platforms, like BeOS (gcc2), EMX on OS/2 (gcc1!), I don't know, Borland C++ for MS-DOS when that guy shows up with a port...those are definitely going to get cut off. This is clearly not crucial, but I did like that fun hacks like this could pop up.

Also note that I think it's reasonable to keep the public headers as C89 in any case, so people can use whatever compiler they want for their own project and still link against SDL.

@flibitijibibo
Copy link
Collaborator

In the FNA community we have something called "VS2010 C", which is basically C90 with stdint. We target this because A: XNA stopped with VS2010 so we just happen to support that exact version, and B: It gives all the obvious features like stdint and flexible comments that the non-MS compilers have supported forever, while also adhering to a lot of the requirements that genuinely old compilers would care about (like variable declarations). It's a very weird and obscure standard to target, but I don't think FAudio or FNA3D have ever gotten bugs from any platform regarding our C standard minimum so it seems to act as a nice compromise between "compatible" and "modern" C.

@Kaktus514
Copy link
Contributor

Also, it would be really nice if SDL had uint_fast16_t or an equivalent for internal use as well as the fixed size types

I think one needs to be a bit careful when using these. Not only do they differ between different platform, they are not necessarily faster than the fixed-sized equivalents. For example, on 64-bit Linux int_fast16_t and int_fast32_t are 64 bits. I don't know in what situation it's actually faster to use 64-bit integers instead of 16-bit or 32-bit on x86_64, perhaps with some hardware or under certain circumstances? But it certainly wastes memory, which could lead to more cache misses, especially if you store a lot of them in some data structure.

One place where I've seen it resulting in poorer performance is in the C++ standard library. The standard random number generators that were introduced in C++11 are defined in terms of these "fast" integer types. This is especially bad with the 32-bit Mersenne Twister (mt19937) which uses uint_fast32_t. On 64-bit Linux this leads to it using twice as much memory than necessary (5 KB instead of 2.5 KB) and it also runs slower according to my own tests.

@sulix
Copy link
Contributor

sulix commented Nov 29, 2022

But if we're talking about moving the whole codebase to stdint types...that's a hard no from me.

On a basic level: literally nothing will ever merge cleanly back into SDL2 if we make a change like that.

While I'd definitely prefer a switch over to the stdint types (that's where my muscle memory lives these days), I do see the point. (And while we could do some slow transition, where we make the public API use stdint types, but keep using the SDL ones under the hood until SDL2 isn't seeing updates anymore, that's ugly in its own ways. IMHO, it's the API's types that are more important — both because we can't break them, and because that's where a difference from "what everyone else is doing" hurts most, with things like bindings generators.)

One thing that's probably worth doing, and explicitly stating (since I think it's already the case), is guaranteeing that — where there is an equivalent stdint type — the SDL definition will be identical, so they can be used interchangably. This is something which hit the Linux kernel's similar definitions, where there were concerns that things like alignment wouldn't match.

There are three things I really want in C99: [declaring variables anywhere, single-line-comments, not policing things].

Those are also the biggest wins in my book. I could add to them:

  • stdint and stdbool types, though we've already covered them.
  • Designated initializers. These are wonderful for building up big structs, though they don't exist (or rather, don't behave the same way) in C++, which also means that older compilers which only support some C99 features are missing them.
  • On the "declaring variables anywhere" front: specifically doing so in for loops.

Most of those are supported even by older compilers because C++ has them: if you drop designated initializers and stdint/stdbool, I'm pretty certain you can get back to gcc ~3ish or ~msvc2003.

So +1 for "MSVC 2010 C" as a compromise.

And relatedly, curl just decided not to move from C89 → C99.

…I don't know, Borland C++ for MS-DOS when that guy shows up with a port…

(I may have hacked together an stdbool.h for Borland C++ 3 yesterday for another project… Of course, even C89 doesn't help you with segmented memory models or tiny variable name length limits. :-P)

@icculus
Copy link
Collaborator

icculus commented Nov 29, 2022

But it certainly wastes memory, which could lead to more cache misses, especially if you store a lot of them in some data structure.

I think the intention is you'd be using these in local variables as for-loop counters, not in arrays and structs, but that even the standard C++ library implementations didn't understand this demonstrates it was an idea destined to backfire.

@slouken
Copy link
Collaborator Author

slouken commented Nov 30, 2022

I think after discussion, we don't plan to move to stdint/stdbool as the datatypes we use in code. This allows us to more cleanly merge changes between SDL2 and SDL3, and makes it possible to support older compilers. Our types will be compatible with those though, so users of the API have the flexibility to use whichever types are most comfortable.

@NekkoDroid
Copy link

NekkoDroid commented Nov 30, 2022

Just a quick question about the integer types: in some places there are still uses of (unsigned) int (for example SDL_Vulkan_GetInstanceExtensions). Will these be replaced with their fixed width equivalents or is the plan keeping them the same?

Might as well also throw in this question while I am at it: I doubt it, but has there been any thoughts of a size_t like type or is there no use for it with over the fixed with types directly?

@slouken
Copy link
Collaborator Author

slouken commented Dec 1, 2022

Just a quick question about the integer types: in some places there are still uses of (unsigned) int (for example SDL_Vulkan_GetInstanceExtensions). Will these be replaced with their fixed width equivalents or is the plan keeping them the same?

I think the intent is to match the underlying APIs. If you have a list of functions you're curious about, let us know and we can review them.

I don't see a specific reason why SDL_Vulkan_GetInstanceExtensions() would use an unsigned vs a sized type. Feel free to enter an issue and we'll look at it for 3.2.0.

Might as well also throw in this question while I am at it: I doubt it, but has there been any thoughts of a size_t like type or is there no use for it with over the fixed with types directly?

A size_t type has come up, but I think the cases where we've wanted to use it we've opted to use a 64-bit type instead, which doesn't have the variable size problem that size_t does, and at least so far is equal to or larger than size_t on all platforms.

@slouken
Copy link
Collaborator Author

slouken commented Jan 10, 2023

@mupfdev, does the N-Gage SDK support // style comments and declaring variables in the middle of a block? Or if not by default, is there a compiler command line option to enable those?

@eloj
Copy link
Contributor

eloj commented Feb 19, 2023

I'm strongly in favor of moving up to at least C99 for SDL3.

Intermingled declarations alone is worth it, plus one-line comments, designated initializers, compound literals, better (variadic) macro support, even tgmath.h can be nice. That's still a 20+ year old standard, so you can hardly argue that it'll leave 'people behind'.

C11/C17 gets _Static_assert, type alignment specifiers, and _Generic, which I think are nice to have, but probably wouldn't have a major impact on the project day to day, like intermingled decls and simple comments.

(I'm excited for C23. Already enjoying __VA_OPT__, and being able to use %b/0b for printing and specifying values in binary. 40+ years, but we got there eventually.)

@nfries88
Copy link

nfries88 commented Feb 22, 2023

I think one needs to be a bit careful when using these. Not only do they differ between different platform

I'll add to this that there's inconsistencies between standard library implementations (and compiler freestanding headers). As you note, Glibc uses wordsize for 16 and 32 bit fast integer types, but musl uses 32-bit for both, newlib tries making them int/unsigned int if they fit and [u]int_least[N]_t otherwise, bionic and uClibc match Glibc, etc.

@mupfdev
Copy link
Contributor

mupfdev commented May 8, 2023

@mupfdev, does the N-Gage SDK support // style comments and declaring variables in the middle of a block? Or if not by default, is there a compiler command line option to enable those?

// style comments are supported, but declaring variables in the middle of a block is not.

The SDK currently uses an old version of GCC, gcc version 2.9-psion-98r2 (Symbian build 540), and I honestly don't know if this can be enabled or not, but I would like to migrate to a newer version of GCC. The latest possible version would be 4.6.4, as support for ARM PE executables was removed in later versions.

I have tried to compile this version before, but failed.

@Daaaav
Copy link
Contributor

Daaaav commented May 8, 2023

@mupfdev GCC 3.0 is apparently the earliest version that supports declaring variables in the middle of a block, so it might just be a small step up: https://gcc.gnu.org/c99status.html

@ccawley2011
Copy link
Contributor

Another possibility is to use a common subset of C99 and C++98, so that older compilers could still use features like // comments and mixed declarations and code even if they don't have C99 support. This should be easy to do using CMake by using set_source_files_properties to set the language.

@cblc
Copy link

cblc commented May 15, 2023

I agree with @mupfdev , although for different reasons, because I don't write for the ngage. I consider C89 support a very good value, because I'm the kind of dude that writes code thinking that it will last decades not only frontwards but also backwards. Personally, I have used some C99 and C11 stuff in the past, but, today I find myself preferring to stick to C89. And yes, I don't write single line comments either... I consider it like a way of saying "I won't write C++ never again in my life" 🤣

@slouken
Copy link
Collaborator Author

slouken commented May 24, 2023

I'm running into this upgrading hidapi to the latest version. Let's update to // comments and mixed declarations. I think this will also make it easier for people wanting to contribute to SDL, and lowering the barrier for entry there is a good thing.

@ccawley2011, @flibitijibibo, @madebr, how do we enable that in CMake?

@madebr
Copy link
Contributor

madebr commented May 24, 2023

/* */ comments are not enforced at the moment.
For the mixed declarations, you just need to remove the block containing checks for -Wdeclaration-after-statement. (L573-L598)
With those lines removed, I can use mixed declarations + // comments without warnings. CMake does not do -std=gnu90.

@madebr
Copy link
Contributor

madebr commented May 24, 2023

Speaking about c90, I will enable strict c90 on the tests.
That way we will not break the code of our beloved users.

@slouken
Copy link
Collaborator Author

slouken commented May 24, 2023

I have the declaration after statement block removed in a pending PR:
1ec10b0

@icculus
Copy link
Collaborator

icculus commented May 25, 2023

I am resisting the urge to go through every file and move all the variables around, because it'll make merging back into SDL2 hard...but I'm absolutely going to start (over)using this in new code. So if you're not absolutely sure, say so now!

(it's okay to say this restriction is lifted only for hidapi, because it presumably isn't going to support the platforms that would have an ancient C compiler anyhow and we can just exclude hidapi completely for those targets. But if we're doing this in SDL itself, I'm 100% using these features like crazy.)

@mupfdev
Copy link
Contributor

mupfdev commented May 25, 2023

Let's update to // comments and mixed declarations.

Okay, I guess it's settled then. No Symbian support in SDL3. 😔

But if it helps the majority, then I don't want to stand in the way of that.

@sezero
Copy link
Contributor

sezero commented May 25, 2023

Let's update to // comments and mixed declarations.

/* */ comments are not enforced at the moment.

Line comments are not a problem, provided they aren't used in public headers.
Even gcc2.95 or VS5 support them unless one disables compiler extensions.
However, using them in public headers will be bad because you can't control
how people will compile their code against our headers.

@madebr
Copy link
Contributor

madebr commented May 25, 2023

Okay, I guess it's settled then. No Symbian support in SDL3. pensive

But if it helps the majority, then I don't want to stand in the way of that.

Where can I download a symbian toolchain (docker),
and where is the symbian SDL port?

Perhaps there are ways to work around.

@mupfdev
Copy link
Contributor

mupfdev commented May 25, 2023

The compiler is downloaded by the toolchain via CMake:

https://github.com/ngagesdk/ngage-toolchain

The Symbian port is labeled "N-Gage", because it specifically targets Symbian S60v1. It's part of SDL2.

This is how I update the precompiled version of SDL2 for the toolchain. Not elegant, but it works. Haven't had the time to implement it in the official CMakeLists.txt yet:

https://github.com/ngagesdk/SDL

//-style comments are perfectly fine, however, mixed declarations are not supported by this version of GCC.

@slouken
Copy link
Collaborator Author

slouken commented May 25, 2023

You mentioned that you could potentially upgrade the version of GCC in the SDK? Mixed declarations are supported in gcc 3.0 and onward, so anything newer than what you have should work.

@madebr
Copy link
Contributor

madebr commented May 25, 2023

It's a bit annoying the ngage sdk downloaded by https://github.com/ngagesdk/ngage-toolchain only runs on Windows, and needs 32-bit cygwin, which is EOL. None of the mirrors provide a 32-bit cygwin1.dll.

I found this (old) and this (recent) effort to get the toolchain building on modern systems.
It looks like a gcc 3.0 port exists.

@slouken
Copy link
Collaborator Author

slouken commented May 26, 2023

I am resisting the urge to go through every file and move all the variables around, because it'll make merging back into SDL2 hard...but I'm absolutely going to start (over)using this in new code. So if you're not absolutely sure, say so now!

(it's okay to say this restriction is lifted only for hidapi, because it presumably isn't going to support the platforms that would have an ancient C compiler anyhow and we can just exclude hidapi completely for those targets. But if we're doing this in SDL itself, I'm 100% using these features like crazy.)

I merged my change, so let's call it. SDL 3.0 will use C89 with // style comments and mixed declarations. We'll leave old code alone so it can be merged with SDL2 easily, but new code is welcome to use the more flexible style. Party on!

@slouken slouken closed this as completed May 26, 2023
@sezero
Copy link
Contributor

sezero commented May 26, 2023

Can we please add some cmake magic to set C99 + extensions (e.g. -std=gnu99) if the compiler's default isn't at least c99 already? Job for @madebr I guess?

@mupfdev
Copy link
Contributor

mupfdev commented May 26, 2023

Just in case anyone here is able to provide some help with C99 support for the N-Gage. Any help is highly appreciated: ngagesdk/ngage-toolchain#9

@icculus
Copy link
Collaborator

icculus commented Jun 20, 2023

Hey, where did we end up with variable declarations in loops? Like, instead of this...

int i;
for (i = 0; i < 10; i++) {}

...we do...

for (int i = 0; i < 10; i++) {}

...was this considered safe?

@slouken
Copy link
Collaborator Author

slouken commented Jun 20, 2023

Yep, just not in any code that is backported to SDL2.

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