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>
This comment references the PR at mozilla/cbindgen#1156.
* Fix: @CCall calls exclusively in LibQiskit.jl * Mention delay method in `QuantumCircuit` docstring (Qiskit#43) * docs: mention delay method in QuantumCircuit docstring * Update src/circuit.jl Co-authored-by: Jim Garrison <jim@garrison.cc> --------- Co-authored-by: Jim Garrison <jim@garrison.cc> * Narrow the Unitful delay type from `Quantity` to `Time` (Qiskit#44) * Narrow the Unitful delay type from `Quantity` to `Time` * Update test_circuit.jl * Add merge_group as trigger for CI workflows (Qiskit#48) * Add note about cbindgen C23 enum PR (Qiskit#46) This comment references the PR at mozilla/cbindgen#1156. * Add Qiskit Ecosystem link to README (Qiskit#47) * Implement `circuit.unitary` and add tests (Qiskit#52) * Implement circuit.unitary and add tests * Update src/circuit.jl Co-authored-by: Jim Garrison <jim@garrison.cc> * validate non-unitary matrices with check_input keyword. --------- Co-authored-by: Jim Garrison <jim@garrison.cc> * redone * new * Regenerate LibQiskit.jl for Qiskit_jll ~2.4.1 * Update generator.jl * ccall: add support for C target * all gp through autogenerated wrappers to use the new C interface. * update libqiskit * fixing errors * Delete gen/stub_cxx/bits/c++config.h * Delete gen/stub_cxx/complex * addressing review comments * generator.jl a Please enter the .ommit message for your changes. Lines starting * Update generator.jl --------- Co-authored-by: Jim Garrison <jim@garrison.cc> Co-authored-by: Jim Garrison <garrison@ibm.com> Co-authored-by: Muhamad Asif <md.asiful.alam@g.bracu.ac.bd>
cbindgen v0.29.3 emits the C23 fixed-type enum syntax for sized enums[1], so in C23 mode pathrs_proc_base_t names the enum rather than uint64_t. CGo maps a C enum to a *signed* Go type, so none of the open-coded ProcBase constants fit in it any more and the package stops compiling: > internal/libpathrs/libpathrs_linux.go:228:22: cannot use > 0xFFFF_FFFE_7072_6F63 (untyped int constant 18446744067006164835) as > ProcBase value in constant declaration (overflows) Our own tree does not show this, because CI is pinned to cbindgen v0.29.2[2], so the checked-in header and the release tarball still typedef uint64_t. Fedora (and presumably other distros) do hit it, because they build the crate with `cargo-c`, which generates the header with its own cbindgen. Fedora rawhide has cbindgen v0.29.4 and gcc 16, which defaults to C23, so libpathrs-devel-0.2.5-2.fc45 ships the C23 spelling and every Go consumer of it fails to build. The workaround there is to force the older language version with `CGO_CFLAGS=-std=gnu17`. Declare the type as plain uint64 instead, which is what the docstring already promises. That also holds up under the direction discussed in [1], where the #if guards would be dropped in favour of an opt-out and the enum spelling would become unconditional. Nothing else has to change: the values were already converted with C.pathrs_proc_base_t() at each call site, and init() already reads the C constants through int64 temporaries because CGo signs those too. [1]: mozilla/cbindgen#1156 [2]: cyphar#383 Signed-off-by: Martin Pitt <martin@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.