Extract core::ffi primitives to a separate (internal) module

[ricci009]

Introduce library/core/src/ffi/primitives.rs

The regex preprocessing for PR #133944 would be more robust if the relevant types from core/src/ffi/mod.rs were first moved to library/core/src/ffi/primitives.rs, then there isn't a need to deal with traits / c_str / va_list / whatever might wind up in that module in the future

r? @tgross35

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[ricci009]

@rustbot author

[ricci009]

I am having issues with the tidy verification ATM. It seems that when I move c_char and c_long to primitives.rs, I get the tidy errors:

tidy error: /library/core/src/ffi/primitives.rs:121: platform-specific cfg: cfg(all(
            not(windows),
            not(target_vendor = "apple"),
            any(
                target_arch = "aarch64",
                target_arch = "arm",
                target_arch = "csky",
                target_arch = "hexagon",
                target_arch = "msp430",
                target_arch = "powerpc",
                target_arch = "powerpc64",
                target_arch = "riscv32",
                target_arch = "riscv64",
                target_arch = "s390x",
                target_arch = "xtensa",
            )
        ))

I guess it does not like that I have platform-specific configuration checks in primitive.rs.

I am not too familiar with tidy and have not found much on the docs but will continue to look into how I can migrate both types to primitives.rs.

[tgross35]

I am having issues with the tidy verification ATM. It seems that when I move c_char and c_long to primitives.rs, I get the tidy errors:

[...]

I guess it does not like that I have platform-specific configuration checks in primitive.rs.

I am not too familiar with tidy and have not found much on the docs but will continue to look into how I can migrate both types to primitives.rs.

Searching for "platform-specific cfg" points here

rust/src/tools/tidy/src/pal.rs

Line 114 in 6c1d960

tidy_error!(bad, "{}:{}: platform-specific cfg: {}", file.display(), line, cfg);

, so I think the new file needs to be added to EXCEPTION_PATHS at the top of that file.

You'll have to move c_char_definition and c_long_definition macros to the new file as well for it to work on its own. And the .md paths in the type_alias! invocations will probably need to get prefixed with ../.

(Fyi ./x c --stage 0 library/core is the quickest way to validate locally, significantly faster than building stage 1 for the rmake test).

[jieyouxu]

Yes, adding the tidy exception is fine.

[ricci009]

I am having issues with the type_alias macro in mod.rs during compilation. Basically, although I insert primitives.rs into mod.rs after the macro type_alias, the compiler still throws the error that the types are missing a stability attribute. I am unsure as to why, any ideas @tgross35 @jieyouxu ?

mod.rs

macro_rules! type_alias {
    {
      $Docfile:tt, $Alias:ident = $Real:ty;
      $( $Cfg:tt )*
    } => {
        #[doc = include_str!($Docfile)]
        $( $Cfg )*
        #[stable(feature = "core_ffi_c", since = "1.64.0")]
        pub type $Alias = $Real;
    }
}

// Primitives contains listed types.
// Contained in seperate file for simple parsing.
mod primitives;
pub use self::primitives::{
    c_char, c_double, c_float, c_int, c_long, c_longlong, c_ptrdiff_t, c_schar, c_short, c_size_t,
    c_ssize_t, c_uchar, c_uint, c_ulong, c_ulonglong, c_ushort,
};

primitives.rs

type_alias! { "c_char.md", c_char = c_char_definition::c_char; #[doc(cfg(all()))] }
[...]

error

error: import has missing stability attribute
  --> library/core/src/ffi/mod.rs:56:5
   |
56 |     c_char, c_double, c_float, c_int, c_long, c_longlong, c_ptrdiff_t, c_sch...
   |     ^^^^^^

[tgross35]

It may be complaining about wanting a stability attribute on pub use, could you try adding one? Or push the updated code and it will be a bit easier to take a look.

[ricci009]

I just did not think it was necessary to add the stability attribute when adding the primitive module as it uses the type_alias to define the stability attributes for each type.

[tgross35]

In a way it is kind of weird since the module is private, but it also makes sense - already-public items may be exported in new modules, unstably. (I had to go through this thought process myself)

You will probably have to split the pub use into two groups so you can have different stability attributes on the reexport, one with core_ffi and one with c_size_t. Seems like the doc paths might also need to be e.g. "c_char.md" -> "../c_char.md".

[ricci009]

I tried the ../ method and it doesn't seem to find the correct path when I was testing. Why could I not keep it how it is ("c_char.md") if primitives.rs is in the same directory as the md files?

[tgross35]

Oh, my bad; I was thinking this moved to a subdirectory, but you're right 👍

[ricci009]

@rustbot ready

[tgross35]

Looks great! Please squash then r=me

[ricci009]

@bors r=tgross35

[bors]

@ricci009: 🔑 Insufficient privileges: Not in reviewers

[ricci009]

@tgross35 not sure if I did the r=me thing correctly but squashed and ready to go!

[tgross35]

r=me is just a convention but doesn’t actually do anything, bors still checks permissions. Thanks!

@bors r+

[bors]

📌 Commit 3419e2f has been approved by tgross35

It is now in the queue for this repository.

[lygstate]

Can we mark this as something like stable_internal for private modue?
Or just #[stable(feature = "", since = "")]

[tgross35]

I'm not sure what you are asking for here; this is the gate for types like https://doc.rust-lang.org/stable/core/ffi/type.c_char.html, which are public.

[lygstate]

I see, I mean this primitives.rs is a private module, there is no need attach stable/unstable attributes on it, maybe always stable is ok, as we already add stable/unstable attributes at

https://github.com/rust-lang/rust/blob/master/library/core/src/ffi/mod.rs#L40-L47

mod primitives;
#[stable(feature = "core_ffi_c", since = "1.64.0")]
pub use self::primitives::{
    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,
};
#[unstable(feature = "c_size_t", issue = "88345")]
pub use self::primitives::{c_ptrdiff_t, c_size_t, c_ssize_t};

So these stable/unstable attributes is redundant, and seems in rust there is no way drop it

[lygstate]

done, I found way to deal with it