Support for Optional arguments

[MeFisto94]

Input C/C++ Header

Note: In the real world, priority is an enum, but I wanted to keep this header minimal, as the outcome is the same.

extern "C" {
    void op_log(const char *message, const int priority = 1, const int line = -1, const char *function = "", const char *file = "");
}

Bindgen Invocation

This is probably irrelevant, but for completeness:

let bindings = bindgen::Builder::default()
        .clang_arg("include/rust_wrapper/wrapper.hpp")
        .clang_arg("-I./openpose/include")
        .clang_arg("-xc++")
        .clang_arg("-std=c++14")
        .parse_callbacks(Box::new(bindgen::CargoCallbacks))
        .generate()
        .expect("Unable to generate bindings");

Actual Results

extern "C" {
    pub fn op_log(
        message: *const ::std::os::raw::c_char,
        priority: op_Priority,
        line: ::std::os::raw::c_int,
        function: *const ::std::os::raw::c_char,
        file: *const ::std::os::raw::c_char,
    );
}

Expected Results

extern "C" {
    pub fn op_log(
        message: *const ::std::os::raw::c_char,
        priority: ::std::Option<op_Priority>,
        line: ::std::Option<::std::os::raw::c_int>,
        function:  ::std::Option<*const ::std::os::raw::c_char>,
        file:  ::std::Option<*const ::std::os::raw::c_char>,
    );
}

Now I am uncertain whether extern "C" supports that, as it might change the ABI, but if that's the case, one could keep op_log private (maybe _op_log) and expose a small op_log wrapper, which only passes the parameters when Some().

Alternatively, one could extract the default values and pass them using said wrapper.

Maybe you have a different idea as well :)

[emilio]

Yeah, that can't work. Option<c_int> is not the same as c_int. There's no way to express this in rust in a simple way, afaict.

We could build different wrappers for different argument counts like:

    pub fn op_log(
        message: *const ::std::os::raw::c_char,
        priority: op_Priority,
        line: ::std::os::raw::c_int,
        function: *const ::std::os::raw::c_char,
        file: *const ::std::os::raw::c_char,
    );

And then:

fn op_log_1(message: *const c_char) {
  op_log(message, -1, -1 ...)
}

Though it's a bit ugly.

[MeFisto94]

I've given that a bit of thought and the problem is: c++ only supports doing the defaults from left to right (assuming the types are equivalent), i.e. if you want to state the file, you have to specify the function as well. This does not translate to Rust's Option<> and we have two possible ways of solveing that and another way:

1. Enforce this by asserting, panicking, erroring when passing like op_log(Some(), None(), Some(), None()).
2. Just deal with it and use the defaults. Actually we have a pretty good example here already: specifying a file but not a line should be as possible as only specifying a function. On the other hand there might be cases where breaking that C++ rule might be unexpected for the code and lead to unexpected behavior in the library. One could argue, that if a library supports defaults, though, that those should always be sane and if not the bindings author should care to fix these things.

Now for 2 there are two possible ways:

1. Generate stubs in c and then call the method with the defaults, extracted by clang from the method signature: That would mean having op_log_10000(message: *const char...) which passes the defaults for all the others, op_log_10001(message: ..., file: ...) etc. This has the problem of generating 2^nb_args stub methods and they all have to be compiled (by default bindgen doesn't compile and link c-stubs in).

2. Call the methods from rust as suggested, translateing the defaults. Good luck with defines, structs and other code in that case. i think this only works for primitive types, which might be most cases, but not the others.

Maybe a hybrid solution would be possible?

[pvdrz]

@MeFisto94 I think 1 is hard as generating c code is not something bindgen can do right now. About 2 I think a sensible idea would be defining a bunch of constants like:

const op_log_default_priority: c_int = 1;
const op_log_default_line: c_int = -1;

So the user can still access the defaults and decide how to expose a better API upwards or something.

What do you think?