search

janet-lang / janet

A dynamic language and bytecode vm
https://janet-lang.org
MIT License
3.48k stars 225 forks source link

You require C11 because you use stdatomic.h #1374

Closed ryandesign closed 5 months ago

ryandesign commented 8 months ago

README.md, CONTRIBUTING.md, the manpage, the Makefile, the meson.build file, all say you use C99, but you include stdatomic.h (#1041) which is C11.

bakpakin commented 8 months ago

So I don't think we really need stdatomic in most cases since we can use the compiler builtins for each platform. As referenced in the original bug, Janet compiles just fine without stdatomic on my GCC/Linux platform and I assume most platforms.

That said, when it comes to compliance bugs like this, I would prefer to have references to actual operating systems/compilers since pure C99 isn't quite as actionable since Janet needs more control of atomics than is offered in C99 to be generally correct across systems.

I'm happy to add some ifdefs for various systems though if there is a particular system that doesn't work.

ryandesign commented 8 months ago

Well, compilers that don't support C11, and even some that do, don't have stdatomic.h.

For example, versions of Apple Clang prior to 700 don't have it even though Apple Clang 500 supports most of the rest of C11.

Trying to build Janet on OS X 10.8 with Apple Clang 503 fails:

https://build.macports.org/builders/ports-10.8_x86_64-builder/builds/163387/steps/install-port/logs/stdio

abstract.c:35:10: fatal error: 'stdatomic.h' file not found
#include <stdatomic.h>
         ^
1 error generated.

For non-Apple (i.e. llvm.org) versions of clang there will be a different cutoff but if you don't need it for any clang then you could exclude those by checking if __clang__ is defined.

GCC will have a cutoff version before which it doesn't have stdatomic.h but if you don't need it there you can exclude it by checking if __GNUC__ is defined. Clang defines that too, so that would take care of both of them.

Since adding stdatomic.h sounds like a workaround for a specific compiler, it might be better to include it only when using that compiler rather than manually excluding all the other ones that don't need it.

bakpakin commented 8 months ago

So going back to the original bug, I'm not even sure why TCC support was added - tcc didn't get atomics support until very recently, and I think the initial implementation has some quirks - I couldn't get it to work with the latest version provided by my distro. I will opt out of including stdatomic.h unless explicitly configured to (JANET_USE_STDATOMIC is defined) since none of the configurations we explicitly support needs it.

bakpakin commented 8 months ago

@ryandesign Let me know if the latest master compiles on your system - when it comes to porting to older systems, there could be other issues as well.