-
Notifications
You must be signed in to change notification settings - Fork 1.1k
ctest: extract bindings from template into source #4551
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
I think it would be a good idea to layout some naming rules from here on out, for example what to name any new tests, whether their C equivalents should have the same name or a slightly modified name, etc. |
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.
Few small things but I think this looks good!
} | ||
let val = {{ ident }}; | ||
let val = {{ const_cstr.rust_ident }}; |
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.
Leave this one as ctest_const_cstr__{{ const_cstr.ident }}
if let syn::Type::Ptr(ptr) = &constant.ty { | ||
let is_const_c_char_ptr = matches!( | ||
&*ptr.elem, | ||
syn::Type::Path(path) | ||
if path.path.segments.last().unwrap().ident == "c_char" | ||
&& ptr.mutability.is_none() | ||
); | ||
if is_const_c_char_ptr { | ||
let item = TestCstr { | ||
test_ident: cstr_test_ident(constant.ident()), | ||
rust_ident: constant.ident().into(), | ||
c_ident: helper.c_ident(constant).into(), | ||
c_type: helper.c_type(constant)?.into(), | ||
}; | ||
const_cstr_tests.push(item) | ||
} | ||
} else { |
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.
This doesn't add any test in the case where it is a pointer but not a CStr
. Either make the top-level if
also check for the pointer type so the nested if
isn't needed:
matches!(
&constant.ty,
syn::Type::Ptr(syn::Type::Path(path))
if path.path.segments.last().unwrap().ident == "c_char"
&& ptr.mutability.is_none()
);
Or bump the MSRV by 1 again and use let chains to do the same thing but slightly cleaner.
pub(crate) test_ident: BoxStr, | ||
pub(crate) rust_ident: BoxStr, | ||
pub(crate) c_ident: BoxStr, | ||
pub(crate) c_type: BoxStr, |
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.
pub(crate) test_ident: BoxStr, | |
pub(crate) rust_ident: BoxStr, | |
pub(crate) c_ident: BoxStr, | |
pub(crate) c_type: BoxStr, | |
pub test_ident: BoxStr, | |
pub rust_ident: BoxStr, | |
pub c_ident: BoxStr, | |
pub c_type: BoxStr, |
Optional nit: if a struct isn't public, pub
(without crate) is fine to get the same visibility as the struct itself
extern "C" { | ||
fn __test_const_{{ ident }}() -> *const {{ ty }}; | ||
fn __{{ constant.test_ident }}() -> *const {{ constant.rust_type }}; |
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.
Same, ctest_const__{{ const_cstr.ident }}
is fine
My feedback isn't too significant, I'm going to merge it then add a followup commit |
Following this up with a few fixes in #4554 |
Description
Extracts out everything except for bindings from the test template, so that each test is parameterized by a single struct, based on this commit.
Sources
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI