Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dynamic loading support #1846

Closed
wants to merge 2 commits into from

Conversation

joechrisellis
Copy link
Contributor

Hi guys,

Following on from issue 1541, here's an implementation for libloading support in bindgen. I'd still consider this a prototype at the minute, so any comments are welcome and appreciated.

Implementation

  • The existing functionality is maintained if dynamic loading mode is not specified.
  • We introduce two new flags -- --dynamic-loading and --dynamic-library-name -- which enable dynamic loading mode and specify a name for the shared library respectively. The 'name' of the shared library is what you want the struct containing the bindings to be called. For example, you could call it BZip2 -- then you'd instantiate the dynamically loaded library with let lib = BZip2::new();. If --dynamic-library-name is not specified it'll just default to Lib.
  • If dynamic loading mode is specified, the bindings are created inside a library struct alongside some libloading boilerplate.

Example

Given the following header.h:

int foo(int x);
void bar(double x);

And the following code inside our build.rs:

let bindings = bindgen::Builder::default()
    .header("header.h")
    // Use dynamic loading
    .dynamic_loading(true)
    .dynamic_library_name("MyLibrary")
    // ...
    .generate()
    .expect("Unable to generate bindings");

We generate the following bindings:

/* automatically generated by rust-bindgen 0.54.1 */

pub struct MyLibrary<'a> {
    foo: libloading::Symbol<
        'a,
        unsafe extern "C" fn(x: ::std::os::raw::c_int) -> ::std::os::raw::c_int,
    >,
    bar: libloading::Symbol<'a, unsafe extern "C" fn(x: f64)>,
}
impl<'a> MyLibrary<'a> {
    pub fn new(lib: &libloading::Library) -> MyLibrary {
        unsafe {
            MyLibrary {
                foo: lib.get("foo".as_bytes()).unwrap(),
                bar: lib.get("bar".as_bytes()).unwrap(),
            }
        }
    }
}

Which we can then use in our code with the following:

#![allow(non_upper_case_globals)]
#![allow(non_camel_case_types)]
#![allow(non_snake_case)]
include!(concat!(env!("OUT_DIR"), "/bindings.rs"));

extern crate libloading;

pub fn main() {
    let lib = libloading::Library::new("/path/to/shared/lib.so").unwrap();
    let library_wrapper = MyLibrary::new(&lib);
    unsafe { (library_wrapper.bar)(123.0) };
}

Multiple Dynamically Loaded Libraries

If you want to use multiple dynamically loaded libraries, the correct approach would be to do this in your build.rs:

let bindings_x = bindgen::Builder::default()
    .header("header_x.h")
    // Use dynamic loading
    .dynamic_loading(true)
    .dynamic_library_name("LibraryX")
    // ...
    .generate()
    .expect("Unable to generate bindings");

let bindings_y = bindgen::Builder::default()
    .header("header_y.h")
    // Use dynamic loading
    .dynamic_loading(true)
    .dynamic_library_name("LibraryY")
    // ...
    .generate()
    .expect("Unable to generate bindings");

let out_path = PathBuf::from(env::var("OUT_DIR").unwrap());
lib1_bindings
    .write_to_file(out_path.join("libx_bindings.rs"))
    .expect("Couldn't write bindings!");
lib2_bindings
    .write_to_file(out_path.join("liby_bindings.rs"))
    .expect("Couldn't write bindings!");

Then do the following in your code:

extern crate libloading;

mod libx {
    include!(concat!(env!("OUT_DIR"), "/libx_bindings.rs"));
}
mod liby {
    include!(concat!(env!("OUT_DIR"), "/liby_bindings.rs"));
}
pub use lib1::LibraryX;
pub use lib2::LibraryY;

(the mod libx { ... } is to prevent namespace collisions if LibraryX and LibraryY have common includes -- e.g. if they both have #include <stdint.h>)


Comments and criticisms absolutely welcome! If this seems like a suitable approach I can update the docs in a separate PR. Cheers! 🍻

@dergroncki
Copy link

I tested the dynamic loading support with one dll file on windows and it worked like a charm! Exactly what I wanted.

Question: Instead of "MyLibrary" as in your example I used "ModelLib", but "ModelLib" is marked as unknown although the program is compiled without errors. Thanks for any hint.

Screenshot 2020-08-02 at 17 32 48

Program extract of the automatically generated code:

extern crate libloading;
pub struct ModelLib<'a> {
    #[doc = ""]
    createInstance: libloading::Symbol<
        'a,
        unsafe extern "C" fn(lengthBillet_m: f64, radiusBillet_m: f64) -> *mut ModelData,
    >,
    destroyInstance:
        libloading::Symbol<'a, unsafe extern "C" fn(data: *mut ModelData) -> ::std::os::raw::c_int>,
    calculate:
        libloading::Symbol<'a, unsafe extern "C" fn(data: *mut ModelData) -> ::std::os::raw::c_int>,
    enableLogging: libloading::Symbol<
        'a,
        unsafe extern "C" fn(
            data: *mut ModelData,
            path: *const ::std::os::raw::c_char,
        ) -> ::std::os::raw::c_int,
    >,
    errorLog: libloading::Symbol<
        'a,
        unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
    >,
    errorLogSize: libloading::Symbol<'a, unsafe extern "C" fn() -> ::std::os::raw::c_int>,
    customer: libloading::Symbol<
        'a,
        unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
    >,
    date: libloading::Symbol<
        'a,
        unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
    >,
    version: libloading::Symbol<
        'a,
        unsafe extern "C" fn(msg: *mut ::std::os::raw::c_char, maxSize: ::std::os::raw::c_int),
    >,
}
impl<'a> ModelLib<'a> {
    pub fn new(lib: &libloading::Library) -> ModelLib {
        unsafe {
            ModelLib {
                createInstance: lib.get("createInstance".as_bytes()).unwrap(),
                destroyInstance: lib.get("destroyInstance".as_bytes()).unwrap(),
                calculate: lib.get("calculate".as_bytes()).unwrap(),
                enableLogging: lib.get("enableLogging".as_bytes()).unwrap(),
                errorLog: lib.get("errorLog".as_bytes()).unwrap(),
                errorLogSize: lib.get("errorLogSize".as_bytes()).unwrap(),
                customer: lib.get("customer".as_bytes()).unwrap(),
                date: lib.get("date".as_bytes()).unwrap(),
                version: lib.get("version".as_bytes()).unwrap(),
            }
        }
    }
}

@joechrisellis
Copy link
Contributor Author

@dergroncki, good to know that it worked for you! If the program compiles just fine, then the 'not found' error you're seeing is probably because your IDE is unable to process the include macros for whatever reason. You could test this by copying the generated bindings.rs file and dumping it into your file in the place of the include macro -- if this doesn't show any errors, then it's probably that.

On another, rather more general note -- I'm planning on spending some time today to get this PR into a production-ready state. At the moment, it's all rather ad-hoc and will occasionally generate invalid bindings for C++ headers. Happy to see that you have found it useful though. 😄

Copy link

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 😃 A few comments/questions

src/lib.rs Outdated
Comment on lines 1732 to 1720
dynamic_loading: bool,

/// The name of the dynamic library if we are generating bindings for a shared library.
dynamic_library_name: Option<String>,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to merge those two options in the commands and the builder? As in have a dynamic_loading argument taking a name value? Or something similar. That way it would be easier to use and implement maybe 😃 If the value is optional that would be even better to default to the library name in that case. Otherwise we can check if the name string is empty or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's a much better idea. I'll do this 😄

Comment on lines 2480 to 2481
// Finally, only generate dynamic bindings for functions (for now! this could
// change in the future to support variables).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I had not considered dynamically loading variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, AFAIK static constants should be dynamically-loadable too, but I haven't thought about this too much. 👍

@goto-bus-stop
Copy link

This looks nice! I'm trying to use it with a two-crate setup, one -sys and one Rusty wrapper. Unfortunately that doesn't work right now because the struct members are private. Should they just all be declared pub by default? I think that would match the non-dynamic bindgen behaviour.

@joechrisellis
Copy link
Contributor Author

This looks nice! I'm trying to use it with a two-crate setup, one -sys and one Rusty wrapper. Unfortunately that doesn't work right now because the struct members are private. Should they just all be declared pub by default? I think that would match the non-dynamic bindgen behaviour.

Yes, you're absolutely right, they should be. This is a trivial fix, will update in just a moment. My bad! 🙂

@nagisa
Copy link
Member

nagisa commented Aug 23, 2020

Hi,

Maintainer of libloading here. This is all super cool. I do have a suggestion in terms of what the generated API should look like. I suggest that instead of generating a structure with public fields you may want to expose the symbols through methods on an inherent implementation instead:

struct SomeName<'a> {
    sym1: Symbol<...>,
    sym2: Symbol<...>,
}

impl SomeName<'a> {
    unsafe extern "C" fn sym1(&self, ...) { /* call self.sym1 */ }
    unsafe extern "C" fn sym2(&self, ...) { /* call self.sym2 */ }
}

Doing it this way is somewhat safer, and more future proof as breaking changes to libloading or bindgen most likely would have no effect on the API most people would use.

Another thing that needs to be triple-checked are the globals (i.e. what conventionally translates to extern "C" { static FOO: ...; }).

@joechrisellis
Copy link
Contributor Author

@nagisa thanks for your excellent suggestions! I will update the patch accordingly. 😄

@joechrisellis
Copy link
Contributor Author

@Michael-F-Bryan would it be okay to adapt some of your ideas for this patch? Specifically, we're interested in doing what @nagisa has suggested above, which is very similar to your work. Just wanted to check with you! 😄

@timfish
Copy link

timfish commented Aug 25, 2020

Many thanks for all the work that's going into getting dynamic support. I've already had to write loads of dynamic bindings manually and it's painful.

@joechrisellis if you're passing the library name to .dynamic_library_name("LibraryY") in build.rs, could you make the path in load_from_path an Option and default to that name is no path is supplied? This would allow users to load from a specific path or pass None to use the OS's default resolution.

So modifying the work from @Michael-F-Bryan :

impl Bindings {
    pub unsafe fn load_from_path<P>(
        path: Option<P>,
    ) -> Result<Self, ::libloading::Error>
    where
        P: AsRef<::std::ffi::OsStr>,
    {
        let path = path.unwrap_or("LibraryY"); // something like this...
        let library = ::libloading::Library::new(path)?;
        ...
    }
}

My dream would be that when I'm creating a *-sys crate, consumers of the crate could switch between dynamic and static with only a feature flag. This would require that both are exposed though the same code paths.

So for dynamic you'd get:

struct SomeName<'a> {
    sym1: Symbol<...>,
    sym2: Symbol<...>,
}

impl SomeName<'a> {
    unsafe extern "C" fn sym1(&self, ...) { /* call self.sym1 */ }
    unsafe extern "C" fn sym2(&self, ...) { /* call self.sym2 */ }
}

and for static you'd get:

struct SomeName;

impl SomeName<'a> {
    unsafe extern "C" fn sym1(&self, ...) { /* call static bindings */ }
    unsafe extern "C" fn sym2(&self, ...) { /* call static bindings */ }
}

Without this, the Rust wrappers for sys crates are going to need some laborious boilerplate abstraction over the top to support both dynamic and static bindings.

I'm not suggesting we modify the default bindgen behaviour, but it would be nice to support this kind of usage.

@joechrisellis
Copy link
Contributor Author

Hi @timfish!

@joechrisellis if you're passing the library name to .dynamic_library_name("LibraryY") in build.rs, could you make the path in load_from_path an Option and default to that name is no path is supplied? This would allow users to load from a specific path or pass None to use the OS's default resolution.

I am a little skeptical of overloading the .dynamic_library_name method -- at the moment, that sets the name of the struct for the generated bindings. The purpose of this is so that we can avoid name collisions if we want to include multiple dynamically-loaded libraries in a single project. This is explained a bit more in the original issue (issue 1541), but I think some of the information there might be slightly outdated now. Unless there is some mechanism/env var like LD_LIBRARY_PATH that can resolve the name of the library (i.e. LibraryY) to a path at runtime that I am unaware of? Let me know if there is! 😄

However, I do like the idea of perhaps passing a path at build-time in build.rs. Something like .dynamic_library_path would work quite well. So, we could do exactly what you suggest, but introduce a new option for it instead of overloading an existing one. Let me know if you think this is the way to go.

My dream would be that when I'm creating a *-sys crate, consumers of the crate could switch between dynamic and static with only a feature flag. This would require that both are exposed though the same code paths.

This is a great idea, although I do think it's probably out-of-scope for this ticket. There might also be some further challenges with doing this. I think that'd work fine for C bindings, but for C++ bindings where we generate structs for classes, we would probably have to find some workaround.

Appreciate your comments, too! Glad to see that people are finding this useful.

@Michael-F-Bryan
Copy link
Contributor

@Michael-F-Bryan would it be okay to adapt some of your ideas for this patch? Specifically, we're interested in doing what @nagisa has suggested above, which is very similar to your work. Just wanted to check with you! smile

Yeah, for sure! The best thing that could happen is someone reuses ideas/code from my prototype and integrates them upstream 🙂

@hug-dev
Copy link

hug-dev commented Aug 27, 2020

One thing I thought about: some dynamic libraries do not implement all of the functions of their corresponding API (I saw this for the PKCS 11 Crypto API for example).
That means that a call to new would error out if one of the function is not implemented.
If you think this is a problem as well, I can propose the following solution: the Library struct could contain Option of function pointers instead of them directly and the new method would set the corresponding one to None if the get call errors out.
What do you think?

@timfish
Copy link

timfish commented Sep 14, 2020

Would it be because more parenthesis are needed 😋?

Probably! 🤦‍♂️ 🤣

But yes, my point was that the methods make the interface a bit simpler to consume!

@Michael-F-Bryan
Copy link
Contributor

Michael-F-Bryan commented Sep 14, 2020

Besides ergonomics, exposing the bindings as methods is required for safety. If the fields were public, it'd be too easy for people to extract the underlying function pointer and accidentally use it after the library is unloaded from memory. That's one of the big reasons libloading uses its own Symbol wrapper.

See nagisa/rust_libloading#13 (comment) from the libloading repo and the linked rust-lang/rust#29625 (comment).

@hug-dev
Copy link

hug-dev commented Sep 14, 2020

Agree that having Symbol seems safer. So one option would be to have only public fields of type Result<Symbol<..>> and the methods to access the functions easily and that also return a Result.

I think also that what proposed @goto-bus-stop might be a good idea:

I don't know what a good solution would be though—adding a method to check for symbol existence might be too much auto-generated API surface for bindgen, and you'd have to deal with the possibility of loaded symbol names clashing with the bindgen-provided method names…

For each function toto there could be two associated methods on the TestLib struct:

  • fn can_call_toto(&self) -> Result<(), libloading::Error>: returning basically self.toto.map(|_| ())
  • fn toto: expected prototype but without returning a Result, directly return the contained value or panic if there was an error when calling get

Then the assumption for callers would be to do something like:

can_call_toto()?;
toto();

Or directly call toto if you are sure!

the possibility of loaded symbol names clashing with the bindgen-provided method names…

Maybe we could address that by having the implementation of the can_call methods inside their own namespace. I believe you can have impl blocks inside submodules. Could even be the can_call module, with the same name as the function:

can_call::toto()?;
toto();

edit: hmm forgot that those are methods on a struct so the namespacing trick will probably not work 🤦

@Michael-F-Bryan
Copy link
Contributor

Agree that having Symbol seems safer.

@hug-dev, keep in mind that libloading::Symbol holds a reference to the original library to prevent accidentally using it after the library is unloaded without runtime overhead. Storing a Symbol in your wrapper struct alongside the libloading::Library would create a self-referential type.

That's why the wrapper's constructor (as currently formulated) would dereference a Symbol to get at the function pointer directly, and store that in the generated type.

Safety is maintained by hiding function pointers behind a method call and making sure the wrapper retains ownership of the Library, making sure functions and pointers to static variables can't outlive the library they came from.

So one option would be to have only public fields of type Result<Symbol<..>> and the methods to access the functions easily and that also return a Result.

You can't make the fields public.

If they are public, it's possible to use the fact that Symbol implements Deref<Target=fn(...)> and that function pointers are Copy to use a function pointer after the library it points into is removed from memory.

For each function toto there could be two associated methods on the TestLib struct:

  • fn can_call_toto(&self) -> Result<(), libloading::Error>: returning basically self.toto.map(|_| ())
  • fn toto: expected prototype but without returning a Result, directly return the contained value or panic if there was an error when calling get

This approach seems to be the most ergonomic. That way you can look-before-you-leap if you know a symbol will always be present, but also perform the check if you need to. I'm guessing the unwrap() call won't add enough overhead to be noticeable considering you are already doing dynamic dispatch with the function call.

To help with namespacing, what about adding a helper struct so you can write my_bindings.can_call().toto()? So the can_call() method returns a helper type that borrows the bindings and has methods with the equivalent of can_call_toto().

In the unlikely event that a library has a can_call symbol, you could keep prepending/appending underscores until there are no ambiguities.

@joechrisellis
Copy link
Contributor Author

@Michael-F-Bryan has made some excellent points above. I also like @hug-dev's idea of implementing a can_call/look-before-you-leap method. Doing this via a helper struct seems like the right approach.

Thinking about the (probably) more common case where all of the symbols in the dynamic library are guaranteed to be defined, I also think it would be useful to have an option that allows us to fail early if a libloading::Error is raised inside the new method for our dynamic loading library type. This way, we can avoid panicking at runtime if something goes wrong early on when we're trying to load the library. I think that this should be the default option; I would guess that most people aren't working with modular/partially-defined libraries, and in that case, they can deal with an interface that is more ergonomical. I was thinking perhaps --fail-early/fail_early, set to true by default, and with some disclaimer that it's possible to get panics if you choose to turn this off? Would love to hear your thoughts on this. 😄

@hug-dev
Copy link

hug-dev commented Sep 15, 2020

Thanks for the explanation, your points make sense and give arguments in favor of having extra helper methods to check the Result only.

I like the idea of the helper struct containing all the can_call_... methods!

I like the idea of the --fail-early option which will also make code generation simpler: the TestLib struct can then directly contain the function pointers and it is no longer need to have the helper struct with the can_call methods. Or they should still be there but always return Ok(()) so that the interface generated does not change with/without the option.

I would agree of adding this option but not sure of what should be the default. If the default is failing late, then a --fail-early option makes sense but if failing early is the default then probably a --fail-late option would be better 😃

@joechrisellis
Copy link
Contributor Author

@emilio hello! I think this is ready for review now -- I think we can add the --fail-early flag in a future PR if you think it's worthwhile. 😄

@emilio
Copy link
Contributor

emilio commented Oct 6, 2020

(sorry for the lag here, I've been a bit too busy lately :/)

I'll try to get to this this week. Thanks for the patience!

@DominicD
Copy link

Hi I am using this (great work btw! 👍 ) and it does work however I have 2 questions/problems.

  1. In the example in the beginning the function dynamic_loading() is called. It seems to me there was some change because in my case it does not exist and it works without calling it.
  2. The bindings are created every time I build. I am not sure if this is related to this pull request or if it is a general bindgen thing? Is there anything I can do to change that?

@joechrisellis
Copy link
Contributor Author

@DominicD thanks for the feedback! To answer your questions:

  1. Things have evolved slightly since this patch was first submitted. The .dynamic_loading({true,false}) builder method has been superseded by .dynamic_library_name("..."). If you specify this option, it'll generate code for dynamic loading. If you don't, it'll run bindgen for your usual static bindings. Thanks for pointing this out though -- it is a good thing to point out for others!

  2. I believe this is a general bindgen thing. FWIW I have heard of some people generating the bindings once (for each platform they need) using bindgen from the CLI. This might be a good choice for you. It has the advantage of quicker build times, (a) bindgen won't need to be run and (b) you won't have to pull in bindgen and its dependencies for a clean build.

Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally got some time to go through this. Really sorry again for the lag.

I think I might be missing something about why you chose this approach rather than something simpler hanging off the existing code generation machinery? Off-hand it seems this patch just takes a whole separate code generation step, while it could (I think) be part of the current machinery quite easily, unless I'm missing something.

.filter(|&(_, item)| {
// If the user has not chosen to do dynamic loading, then we have nothing to
// do.
if !self.options().dynamic_loading {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this check should really go at the beginning of the function rather than being done for every item.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quite right -- will fix.

// If there is a whitelist and this function is not on it, don't add it.
if !self.options().whitelisted_functions.is_empty() &&
!self.options().whitelisted_functions.matches(&name)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect this to be accounted for via the regular whitelisting mechanism, why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think this can be simplified. I think I had the wrong idea of the relationship between items, codegen_items, and whitelisted_items. I've changed this to iterate through self.items(), but filter out anything whose item ID is not inside codegen_items. It seems to do the job -- is that what you had in mind?

// If there is a blacklist and this function is on it, don't add it.
if !self.options().blacklisted_functions.is_empty() &&
self.options.blacklisted_functions.matches(&name)
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same about this.


// We don't want to include the root module in the list of items to generate
// dynamic bindings for.
if item.id() == self.root_module {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're only generating functions so this check is redundant, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, great spot.

src/lib.rs Outdated
mut self,
dynamic_library_name: T,
) -> Self {
self.options.dynamic_loading = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need to keep track of self.options.dynamic_loading? It seems you really just need a library name, so the dynamic_loading checks could just be self.options.dynamic_library_name.is_some().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed this.

@@ -3948,11 +3959,45 @@ pub(crate) fn codegen(
&(),
);

// If the set of items to generate dynamic bindings for is nonempty...
if !context.dyngen_items().is_empty() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically... This is just effectively generating all the functions right now, is that right?

The way I'd have approached this would be something like, maybe:

  • Add an extra field to CodegenResult (dynamic_functions? / whatever)
  • Handle this in CodeGenerator for Function (just something like if ctx.options().dynamic_loading { /* generate appropriate things */ }, and instead of pushing to the regular CodegenResult field, push it to the dynamic functions, which you'd then handle here.

Is there any reason such an approach wouldn't work? It'd handle a lot more edge cases than this (template functions, etc, which this patch doesn't handle), and should also be much simpler / not need extra ItemSets.

Joe Ellis and others added 2 commits October 27, 2020 18:17
Co-authored-by: Michael-F-Bryan <michaelfbryan@gmail.com>
Copy link
Contributor

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the lag, didn't notice this was updated :(. Feel free to ping next time as needed, I get way too much github mail...

This looks basically good to me, thank you! I have some relatively minor nits, but I think with those addressed this looks good to me.

Given we need to do a breaking release anyhow for f221479, if this is updated soonish we can get it into 0.56.

Or alternatively, if you don't have the time, let me know and I can fix up the review comments myself.

The other great thing to do would be to have a documentation section in the book about this, how to use it and so on.

I don't think I want to block on that but I think it'd be great to add if any of you have the cycles.

Thanks again and sorry for the lag.

@@ -4356,6 +4403,35 @@ mod utils {
args
}

pub fn fnsig_argument_identifiers(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you document what this returns? The signature is not super-self-descriptive.

@@ -3948,11 +3978,28 @@ pub(crate) fn codegen(
&(),
);

if context.options().dynamic_library_name.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if let Some(ref lib_name) = context.options().dynamic_library_name {.

Then use that instead of unwrapping around.

"Check",
context.options().dynamic_library_name.as_ref().unwrap(),
]
.join(""),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just format!("Check{}", lib_name)?

}
}

pub fn add_function(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just push in order to match the result function name?

init_fields: Vec<proc_macro2::TokenStream>,
}

impl Default for DynamicItems {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Derive this?

ret: proc_macro2::TokenStream,
ret_ty: proc_macro2::TokenStream,
) {
assert_eq!(args.len(), args_identifiers.len());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, this seems to assert on varargs parameters.

Not sure if those are not supported widely and this is expected? At least things seem to work when I remove the assert. The following snippet will trigger the assert.

int thing(int, ...);

@emilio emilio closed this in fc5fa9a Nov 25, 2020
@emilio
Copy link
Contributor

emilio commented Nov 25, 2020

I pushed this with my review comments addressed. Will send a follow-up for the var-args issue mentioned by @jsimmons above.

@emilio
Copy link
Contributor

emilio commented Nov 25, 2020

Thanks for all your work @joechrisellis (and everyone else who contributed and helped flesh out issues!)

@hug-dev
Copy link

hug-dev commented Nov 25, 2020

Nice!! 🥳 🥳 🥳
Thanks @joechrisellis , @emilio and everybody involved!

@emilio
Copy link
Contributor

emilio commented Nov 25, 2020

#1931 addresses the varargs functions (requires a bit of an API change but I think it's fine and the result is simpler).

LoganBarnett pushed a commit to LoganBarnett/rust-bindgen that referenced this pull request Dec 2, 2023
Closes rust-lang#1541.
Closes rust-lang#1846.

Co-authored-by: Michael-F-Bryan <michaelfbryan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet