Use C++ fixed-type enumeration syntax under C23 (or higher) as well#1156
Use C++ fixed-type enumeration syntax under C23 (or higher) as well#1156garrison wants to merge 5 commits into
Conversation
C23 [has introduced support for enumerations with fixed underlying type](https://open-std.org/JTC1/SC22/WG14/www/docs/n3030.htm), matching the existing C++ syntax for this functionality. This PR modifies cbindgen to output a header that enables this syntax if either `__cplusplus` is defined (as before) or if the compilation is according to C23 or later (added in this PR). My motivation for this change is that my toolchain complained that some names were defined twice, both as an enum and a typedef. I [fixed the error](https://github.com/Qiskit/Qiskit.jl/blob/73bfbb9440168dc76685632f9fb3d00d3a17bd79/gen/generator.jl#L16-L18) by defining `__cplusplus` even thought the compiler is compiling in C mode. This change will allow me to remove that workaround, as long as C23 or later is set as the C standard.
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
You probably need to update tests right?
Indeed -- and now everything passes locally. |
emilio
left a comment
emilio
left a comment
There was a problem hiding this comment.
LGTM, I'll probably manually squash it once CI is green because there's a couple merge commits. Thanks!
This comment references the PR at mozilla/cbindgen#1156.
| write!(out, "#if {cond}"); | ||
| out.new_line(); | ||
| write!(out, " : {prim}"); | ||
| out.new_line(); | ||
| write!(out, "#endif // {cond}"); | ||
| out.new_line(); |
There was a problem hiding this comment.
Unfortunately, unconditionally outputting this causes issues with some tools that parse header files.
In particular, Python's cffi has very limited support for preprocessor macros and does not support #if conditionals at all. For some conditionals this is trivial to work around (such as header guards -- you can remove them before getting cffi to parse them) but conditionals like this do not have a nice workaround.
If you try to use them, you get this error:
>>> import cffi
>>> ffi = cffi.FFI()
>>> ffi.cdef("#if FOO >= 100\n#endif\n")
cffi.CDefError: cannot parse "#if FOO >= 100"
<cdef source string>:1:1: Directives not supported yetcyphar/libpathrs#382 is an example of how this can cause build failures in a downstream project -- our build scripts strip out any non-#define macros (which is the only thing cffi supports, and even then it's quite limited) so the error you actually get looks more like:
cffi.CDefError: cannot parse ": uint64_t"
<cdef source string>:22:3: before: :
But the core issue is the same. It would be nice if this output could be made configurable so that we can continue to use cbindgen-generated headers with cffi. Otherwise we will have to pin an older version...
There was a problem hiding this comment.
I would be ok with reviewing a patch to make this opt-in / opt-out.
Though, have you considered unifdef perhaps? I think that'd work?
There was a problem hiding this comment.
That's not a bad idea (and I must admit I hadn't considered it) -- unfortunately shelling out to unifdef from the setup script is a little dodgy, it might not work for some packaging setups (distros would probably be okay with the workaround but I think it would be problematic for Python packaging and people doing local from-source installs).
It would be easier to implement that in Python, at which point it might actually end up being less effort to just add support for basic #if/#ifdef conditionals in cffi...
I would be ok with reviewing a patch to make this opt-in / opt-out.
What would you think about a patch which let you specify a C version to generate (to not make this switch too special-purpose) and so if you pick C99 mode (for instance) you don't get these #if conditionals but if you pick C23 mode you do? (To be fair, reading it back this is a little funky -- if you are picking C23 mode you wouldn't need the conditional. Maybe a "max" and "min" C version? 🤷.)
Or would you prefer something more like an option for "limited C" and "full-on C"? (That would make things easier for me in the future, I must admit! 😅)
There was a problem hiding this comment.
I think something like allow_fixed_size_c_enums or something, defaulting to true and avoiding the ifdefs would be a bit more consistent with how we deal with other features like constexpr in c++.
It'd be a breaking change but it'd remove the ifdefs altogether from the generated code? I guess the ifdef route was taken here mostly because we already had a __cplusplus if def for C++-compatible-C but...
| if config.language == Language::C { | ||
| out.new_line(); | ||
| out.write("#if __STDC_VERSION__ >= 202311L"); | ||
|
|
||
| out.new_line(); | ||
| write!( | ||
| out, | ||
| "{} enum {} {};", | ||
| config.language.typedef(), | ||
| tag_name, | ||
| tag_name | ||
| ); | ||
|
|
||
| out.new_line(); | ||
| out.write("#else"); | ||
| } | ||
|
|
||
| out.new_line(); | ||
| write!(out, "{} {} {};", config.language.typedef(), prim, tag_name); | ||
|
|
||
| if config.language == Language::C { | ||
| out.new_line(); | ||
| out.write("#endif // __STDC_VERSION__ >= 202311L"); | ||
| } |
There was a problem hiding this comment.
And this change has a very similar issue -- I believe this will cause multiple-defines errors if our build script strips out the #if directives.
cbindgen v0.29.3 added support for C23 fixed-type enum syntax unconditionally (mozilla/cbindgen#1156) -- it is gated with #if guards but Python's cffi cannot handle those and so our Python bindings would not build. So, pin the newest release without this feature to make our CI green again (we did not pin the cbindgen version used in our CI so the new update also broke our CI). Signed-off-by: Aleksa Sarai <aleksa@amutable.com>
3 participants
C23 has introduced support for enumerations with fixed underlying type, matching the existing C++ syntax for this functionality.
This PR modifies cbindgen to output a header that enables this syntax if either
__cplusplusis defined (as before) or if the compilation is according to C23 or later (added in this PR).My motivation for this change is that my toolchain complained that some names were defined twice, both as an enum and a typedef. I fixed the error by defining
__cpluspluseven thought the compiler is compiling in C mode. This change will allow me to remove that workaround, as long as C23 or later is set as the C standard.