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

[WIP] Import C types from the cty crate #1285

Closed
wants to merge 34 commits into from
Closed

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Feb 24, 2019

So this is more or less how solving japaric/cty#13 and japaric/cty#14 would look like. I've kept the whole history of the cty crate.

I think that I found some bugs in both libc and cty in the process.

cc @japaric @Amanieu @retep998 @alexcrichton @denzp

japaric and others added 30 commits May 24, 2017 19:36
Fix mixed signed / unsigned types
change c_{,u}long to match libc v0.2.23 definitions
Copied a few extra definitions from the libc crate.
Include additional types from libc

Copied a few extra definitions from the libc crate.
11: Re-export core::ffi::c_void r=japaric a=jeikabu

japaric/cty#10

Without this trying to use bindgen like:
```rust
let bindings = bindgen::Builder::default()
        .header("wrapper.h")
        .ctypes_prefix("cty")
        .use_core()
        .generate();
```

Results in lots of:
```
error[E0308]: mismatched types
  --> runng/src/protocol/mod.rs:49:43
   |
49 |         let res = nng_setopt(socket, opt, topic_ptr, topic_size);
   |                                           ^^^^^^^^^ expected enum `cty::c_void`, found enum `std::ffi::c_void`
   |
   = note: expected type `*const cty::c_void`
              found type `*const std::ffi::c_void`
```


Co-authored-by: jake <jeikabu@gmail.com>
@rust-highfive
Copy link

@gnzlbg: no appropriate reviewer found, use r? to override

// FIXME: ??
pub type wchar_t = i32;
} else {
pub type wchar_t = u32;
Copy link
Member

Choose a reason for hiding this comment

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

Windows also supports aarch64 and arm where wchar_t is still u16.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think wchar_t is just too OS and target-dependent to try to shoe-horn it here.

@alexcrichton
Copy link
Member

I would personally pretty strongly discourage this. The libc crate is a fundamental crate in the Rust ecosystem which is designed by RFC and led by the libs team. The same does not apply to the cty crate.

Duplication may be unfortunate but it doesn't matter except for the c_void type really, otherwise all other types are just typedefs which can already unify across the two crates.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 25, 2019

After doing this the feeling I got is that it makes more sense to keep both of these crates separate (not change libc to re-export cty), though I'd prefer to host the cty crate here so that we can use libc's CI to make sure its types are correct, both by using ctest to ensure that in the same platforms that libc's support, and maybe also by adding a build test for all targets that makes sure that both crates implement the same API on all targets that libc supports.

I don't think any change here is going to be RFC worthy. The Unsafe Code Guidelines is already documenting that some Rust types exactly match some C types (e.g. size_t, intptr_t, ptrdiff_t, int16_t, etc.), and all of that will be RFC'ed sometime this year. If that gets merged, it will make more sense to just add these types to core::ffi directly, as in, isize is required by libcore, and if it is guaranteed to match some C type, we can just add that there.

@alexcrichton
Copy link
Member

In terms of hosting I don't think it matters too much where it lives, but I think structurally these should continue to be separate crates for the time being (although an RFC could change that)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Feb 25, 2019

So i'm closing this to submit a different PR that does that, don't want to lose this branch because it discovered a couple of bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants