mozilla / cbindgen

A project for generating C bindings from Rust code
Mozilla Public License 2.0
2.27k stars 294 forks source link

Va list binding compatibility #970

Closed gerard-ryan-immersaview closed 1 month ago

gerard-ryan-immersaview commented 1 month ago

From what I can tell a va_list should be created by calling va_start and no documentation that I could find specifies the use of va_list as an argument explicitly. Assuming this works, I imagine that some compilers happen to implement va_list and ... the same, but ... seems the be the specified way.

I haven't done any runtime testing of these changes, so if this is wrong I'd love to know why.

Sources:

Related: https://github.com/mozilla/cbindgen/pull/968

Closes: https://github.com/mozilla/cbindgen/issues/891

emilio commented 1 month ago

Tests are failing because:

Output failed to compile: Output { status: ExitStatus(unix_wait_status(256)), stdout: "", stderr: "/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:6:32: error: ISO C requires a named argument before ‘...’\n    6 | typedef int32_t (*VaListFnPtr)(...);\n      |                                ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:11:18: error: ISO C requires a named argument before ‘...’\n   11 |   int32_t (*fn1)(...);\n      |                  ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:22:22: error: ISO C requires a named argument before ‘...’\n   22 | int32_t va_list_test(...);\n      |                      ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:24:23: error: ISO C requires a named argument before ‘...’\n   24 | int32_t va_list_test2(...);\n      |                       ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:26:37: error: ISO C requires a named argument before ‘...’\n   26 | void va_list_fn_ptrs(int32_t (*fn1)(...),\n      |                                     ^~~\ncc1: note: unrecognized command-line option ‘-Wno-return-type-c-linkage’ may have been intended to silence earlier diagnostics\n" }

Let's just add a dummy char* or somesuch argument to the function types?

In general this looks good. Could you clean up the commit history to avoid having reverts and so on if you don't mind? Thanks.

emilio commented 1 month ago

Also, in the commit messages and PR title, please use compatibility rather than compatability? I think the later is a typo (but not an english speaker so icbw)

gerard-ryan-immersaview commented 1 month ago

Tests are failing because:

Output failed to compile: Output { status: ExitStatus(unix_wait_status(256)), stdout: "", stderr: "/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:6:32: error: ISO C requires a named argument before ‘...’\n    6 | typedef int32_t (*VaListFnPtr)(...);\n      |                                ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:11:18: error: ISO C requires a named argument before ‘...’\n   11 |   int32_t (*fn1)(...);\n      |                  ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:22:22: error: ISO C requires a named argument before ‘...’\n   22 | int32_t va_list_test(...);\n      |                      ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:24:23: error: ISO C requires a named argument before ‘...’\n   24 | int32_t va_list_test2(...);\n      |                       ^~~\n/home/runner/work/cbindgen/cbindgen/target/package/cbindgen-0.26.0/tests/expectations/va_list.compat.c:26:37: error: ISO C requires a named argument before ‘...’\n   26 | void va_list_fn_ptrs(int32_t (*fn1)(...),\n      |                                     ^~~\ncc1: note: unrecognized command-line option ‘-Wno-return-type-c-linkage’ may have been intended to silence earlier diagnostics\n" }

Let's just add a dummy char* or somesuch argument to the function types?

Oh, I was wondering why this was compiling before, I just assumed it was using c23 like I was, but regardless, I fixed this now.

In general this looks good. Could you clean up the commit history to avoid having reverts and so on if you don't mind? Thanks.

I squashed all the commits except the code move commit because it makes the diffs nicer.