Open bztsrc opened 1 year ago
Hmm, interesting. The CI test builds also compile with -Werror, my guess is that this problem only exists on some Linux distros.
It definitely makes sense to add the define to sokol_app.h (and also sokol_time.h since that also calls clock_gettime).
Can you share some more details about your operating system setup? I'd like to figure out why that problem didn't show up yet.
Ok, I did a little digging. It looks like the problem appears when the headers include features.h (so this is totally operating system independent). I was using -std=c99
, which defines nothing, hence the need for the manual POSIX define. On the other hand, all of your test cases compile using -std=gnu99
, which implicitly defines _GNU_SOURCE
. Now with that define when features.h is included, it also defines all possible POSIX defines, XOPEN extensions as well as GNU extensions.
https://github.com/bminor/glibc/blob/master/include/features.h#L201
That's why it's working for you. So -std=gnu99
command line flag indirectly defines _POSIX_C_SOURCE
, while -std=c99
does not.
Ok, I guess it's up to you now. You can add this define, or you could put an emphasis in the readme on gnu99
needed. As long as I'm concerned, this issue is resolved and could be closed.
Cheers, bzt
ps: I forget to tell you, thank you for this awesome header-only library!
Ok thanks for the investigation, mentioning c99 vs gnu99 somehow rings a bell, I might have stumbled over the issue a long time ago, maybe in the context of Oryol, but then forgot about it again.
I think we should support the -std=c99
case (although technically most sokol headers are currently written in the "common C/C++ subset", because some people prefer to compile the code in C++ mode, so at some point there may be a collision between the "pure C" vs "common C/C++ subset" philosophy. When that happens I think I'll rather ditch C++ mode though, since this is already a bit of a hassle to support.
One thing I'm not sure about is whether the _POSIX_C_SOURCE
is supposed to be put into headers, since it might collide with requirements of other libraries (although, I guess putting it into the implementation section of the sokol headers should be fine - that would be comparable to putting it into a .c source file before the C stdlib includes).
One thing I'm not sure about is whether the _POSIX_C_SOURCE is supposed to be put into headers, since it might collide with requirements of other libraries
I don't think so. It just selects which prototypes are defined from the libc headers. BTW, you do have this define with -std=gnu99
anyway. But maybe defining _GNU_SOURCE
in the sokol headers would be more appropriate? Then it wouldn't matter if -std=c99
or -std=gnu99
passed, it would work exactly the same way like now.
I guess putting it into the implementation section of the sokol headers should be fine
Watch out! You have to define this before you include any of the libc headers!
For example, this won't work:
#include <string.h>
#define _POSIX_C_SOURCE 199309L
#include <time.h> /* you won't get clock_gettime() prototype in this case */
That's because string.h includes bits/libc-header-start.h, which in turn includes features.h. So it doesn't matter if you set the POSIX define after that, because when time.h tries to include features.h, _FEATURES_H will be already defined, so it won't do its magic.
Cheers, bzt
Watch out! You have to define this before you include any of the libc headers!
Hmm ok, the declaration section of the sokol headers are supposed to only include stdint.h, stdbool.h and stddef.h, need to check if those are also affected.
But thinking more about this, it will be a problem. Nothing prevents the user from including any of the problematic C headers before including the sokol header implementations, which then would make any define inside the sokol headers useless, bah.... :(
Maybe we can rather add something along the lines of:
#if defined(C99) && defined(LINUX) && !(_POSIX_C_SOURCE > xxx)
#error "please define _POSIX_C_SOURCE..."
#endif
...so instead of attempting a "magic fix" instead let the user know what needs to be done?
(because in this case, the cleanest fix would be to put the define into the build system, so that everything is automatically compiled with the same _POSIX_C_SOURCE define)
PS: it's basically the same problem as _CRT_SECURE_NO_WARNINGS
on Windows, this also needs to be defined before any other C stdlib header, otherwise it won't have any effect.
Nothing prevents the user from including any of the problematic C headers before including the sokol header implementations
You're right about that.
Maybe we can rather add something along the lines of:
I believe it would be better to have something like
#if defined(__unix__)
# if !defined(_FEATURES_H) && !defined(_POSIX_C_SOURCE)
# define _POSIX_C_SOURCE 199309L
# elif !defined(_POSIX_C_SOURCE) || _POSIX_C_SOURCE < 199309L
# error "Please define _POSIX_C_SOURCE or _GNU_SOURCE or use -std=gnu99
# endif
#endif
What do you think? I've replaced the Linux check with a UNIX check (because this applies to BSDs too), and also check if the user has included any stdlib headers beforehand. If he wasn't, then it simply defines the define and go on. Otherwise there's an extra check when to report an error. I think this is now bullet-proof.
Maybe the whole POSIX define should be replaced by the GNU define? Not sure if you want to force GNU extension, but if that's not a problem, then the whole thing becomes much simpler:
#if defined(__unix__)
# if !defined(_FEATURES_H) && !defined(_GNU_SOURCE)
# define _GNU_SOURCE 1
# else
# error "Please define _GNU_SOURCE or use -std=gnu99
# endif
#endif
it's basically the same problem as _CRT_SECURE_NO_WARNINGS on Windows, this also needs to be defined before any other C stdlib header, otherwise it won't have any effect.
I never had issue with that one, but I agree, sounds pretty similar.
Cheers, bzt
I prefer the _GNU_SOURCE version. Do you want to provide a PR for sokol_app.h and sokol_time.h which adds those blocks?
(PS: not sure what happens on macOS, technically it's a BSD, but I never had problems there with the default settings, but we'll see what happens in to PR's CI pipeline I guess)
I'm trying to create a bare minimum sokol app, but it gives me a warning (which is a problem for me because I compile everything with
-Werror
).The reason is, time.h isn't included correctly. Here's the error:
I can workaround that by specifying the required POSIX define, but I believe this should be done by sokol_app.h.
Here's my source in its entirety:
I compile this with the command:
Compiler: gcc (GCC) 13.1.1 20230429 Glibc: 2.37-3 Arch: x86_64
To reproduce the issue, just comment out the first
_POSIX_C_SOURCE
define, and it won't compile any more.Suggested solution: add this at the beginning of sokol_app.h:
Cheers, bzt