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

Remove core ffi primitive redefinitions #4339

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

highjeans
Copy link

Description

This PR removed the duplicate definitions of the c primitive types and re-exports the definitions from rust's core::ffi module.
Closes #4257

Sources

No API changes as the definitions of c primitive types are now coming from rust's core::ffi module.

Checklist

  • Relevant tests in libc-test/semver have been updated
  • No placeholder or unstable values like *LAST or *MAX are
    included (see #3131)
  • Tested locally (cd libc-test && cargo test --target mytarget);
    especially relevant for platforms that may not be checked in CI

@rustbot
Copy link
Collaborator

rustbot commented Mar 18, 2025

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@highjeans
Copy link
Author

Seems like the failure is related to #94501. I will look into and see if I can fix the issue.

@highjeans
Copy link
Author

Turns out core::ffi:c_* types were unstable in rust 1.63.0 and was only stabilized in 1.64.0. @tgross35 Would you have any ideas on what to do about this?

@tgross35
Copy link
Contributor

This will have to hold off until we do our next MSRV bump, which will likely be later this year. Technically the main branch doesn't need to test down to our current MSRV since 1.0 will require a higher version, but I also don't want to change this CI job in the interest of keeping our branches as similar as possible (for my sanity doing backports).

We do want this so feel free to get it ready, but for now:

@rustbot blocked

Comment on lines +192 to +195
pub use core::ffi::{
c_char, c_double, c_float, c_int, c_long, c_longlong, c_schar, c_short, c_uchar, c_uint,
c_ulong, c_ulonglong, c_ushort,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we could actually go a step further here; we won't want to publicly reexport these types at all anymore since users have access to core::ffi directly, so this block can be removed. Instead,

libc/src/macros.rs

Lines 83 to 86 in 5d549b2

pub(crate) use crate::{
c_char, c_double, c_float, c_int, c_long, c_longlong, c_short, c_uchar, c_uint,
c_ulong, c_ulonglong, c_ushort, c_void, intptr_t, size_t, ssize_t, uintptr_t,
};
can be adjusted to reexport core::ffi for our internal use.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes this is a good point! I will add this right now.

Copy link
Author

Choose a reason for hiding this comment

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

Turns out doing this will break the libc tests as those rely of the re-exports through libc rather than directly from core::ffi. Should I see if I can get the tests to use core::ffi instead of libc for these types?

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems reasonable to me 👍. It could even be a separate PR to merge sooner since the rest of this is blocked

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.

Remove the libc version of core::ffi primitives
3 participants