-
Notifications
You must be signed in to change notification settings - Fork 1.2k
ctest: add suport for C enum #4658
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
Conversation
|
The signededness of a C enum is implementation defined, so should that test be skipped for all C enums? |
ctest-next/src/generator.rs
Outdated
| /// let mut cfg = TestGenerator::new(); | ||
| /// cfg.c_enum(|e| e == "pid_type"); | ||
| /// ``` | ||
| pub fn c_enum(&mut self, f: impl Fn(&str) -> bool + 'static) -> &mut Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this take &Type rather than &str? To match rename_alias.
Maybe the name alias_is_c_enum would be a bit more descriptive, there are a handful of ways to represent C enums https://mdaverde.com/posts/rust-bindgen-enum/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can't, because it's also used to filter out constants with the same type. The only way to do it would be to create a Type struct in that method, but then the public and ty fields would be incorrect.
Could you add a test in |
|
If I use an unsigned type in |
|
I think that's actually an important catch; the fallback here Line 279 in 2e670a8
|
Since it piggybacks on tests for constants, incorrect item values would be caught with that. As for the repr type I'm not sure how to catch that. If it uses too small a repr size the constant test would overflow and catch it. For now |
8a40344 to
910290e
Compare
|
FreeBSD is strange. A lot of the times it segfaults on |
tgross35
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signededness of a C enum is implementation defined, so should that test be skipped for all C enums?
repr(C)should generally match. I know there might be some problems with that, but have you run into any here?
Could you add a test inctest-testto ensure this catches incorrect item values and things like the wrong repr type?Since it piggybacks on tests for constants, incorrect item values would be caught with that. As for the repr type I'm not sure how to catch that. If it uses too small a repr size the constant test would overflow and catch it. For now
ctest-testis only run on one platform, but on others a different type would have to be used.
I think this could look something like this:
pub type enum_repr_too_small = u16;
pub const ENUM_REPR_TOO_SMALL_A: enum_repr_too_small = 0;
#[cfg(windows)]
pub type enum_wrong_signedness = i32;
#[cfg(not(windows))]
pub const ENUM_WRONG_SIGNEDNESS_A: enum_wrong_signedness = 0;mapping to C:
enum enum_repr_too_small {
ENUM_REPR_TOO_SMALL_A;
}
enum enum_wrong_signedness {
ENUM_WRONG_SIGNEDNESS_A;
}And then set these as alias_is_c_enum. Then the test at
Line 39 in 10d68a4
| fn t2() { |
src/macros.rs
Outdated
| (@ty_win $repr:ty) => { $repr }; | ||
| (@ty) => { $crate::c_uint }; | ||
| (@ty_win) => { $crate::c_int }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using different macro arms, add a pub(crate) type CEnumRepr to src/types.rs and use it here.
This will also simplify the tests, you can do e.g. 0 as CEnumRepr rather than splitting on Windows
4bc09f1 to
d212197
Compare
Description
Adds the ability to specify whether a type alias and a set of constants that use that type alias maps to a C enum, as well as the ability to skip them.
Sources
Checklist
libc-test/semverhave been updated*LASTor*MAXareincluded (see #3131)
cd libc-test && cargo test --target mytarget);especially relevant for platforms that may not be checked in CI