ctest-next: miscellaneous filtering bug fixes#4613
Conversation
rustbot
added
A-CI
mbyx
force-pushed
the
ctest-next-bug-fix
branch
3 times, most recently
from
51cf365 to
d1382d3
Compare
mbyx
force-pushed
the
ctest-next-bug-fix
branch
3 times, most recently
from
9d9d43f to
e96f64c
Compare
| filtered_ffi_items: FfiItems, | ||
| ffi_items: &'a FfiItems, |
There was a problem hiding this comment.
Does this need to keep the unfiltered ffi_items at all anymore? It seems cleaner to only keep the filtered items here, if possible.
There was a problem hiding this comment.
It's to fix one of the bugs, if say a struct T1Bar is skipped for testing it won't be in filtered_ffi_items, but if some other test for a different struct returns a *const T1Bar, then it would fail to realize that T1Bar is a struct, since originally we were using the filtered_ffi_items in c_type.
mbyx
force-pushed
the
ctest-next-bug-fix
branch
2 times, most recently
from
38c56f1 to
a0c27ab
Compare
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
For the tests I left a few comments at #4600 (comment) before realizing they were part of this PR, could you apply them here?
ctest-next/src/template.rs
Outdated
| let skipped: Vec<_> = self | ||
| .filtered_ffi_items | ||
| .$field | ||
| .extract_if(.., |item| { | ||
| self.generator | ||
| .skips | ||
| .iter() | ||
| .any(|f| f(&MapInput::$variant(item))) | ||
| || (self.generator.skip_private && !item.public) | ||
| }) | ||
| .collect(); | ||
| if verbose { | ||
| skipped | ||
| .iter() | ||
| .for_each(|item| eprintln!("Skipping {} \"{}\"", $label, item.ident())); | ||
| } |
There was a problem hiding this comment.
No need to collect to an intermediate Vec:
let skipped = self
.filtered_ffi_items
// ...
.extract_if(/* ... */);
for item in skipped {
if verbose {
eprintln!("...");
}
}Also, put the self.generator.skip_private && !item.public check before checking self.generator.skips, so it doesn't need to test all the skips if there is an easy return.
| // For structs/unions/aliases, their type is the same as their identifier. | ||
| MapInput::Alias(a) => (a.ident(), cdecl::named(a.ident(), cdecl::Constness::Mut)), | ||
| MapInput::Struct(s) => (s.ident(), cdecl::named(s.ident(), cdecl::Constness::Mut)), | ||
| MapInput::Union(u) => (u.ident(), cdecl::named(u.ident(), cdecl::Constness::Mut)), | ||
| // FIXME(ctest): For some specific primitives such as c_uint, they don't exist on the | ||
| // C side and have to be manually translated. If they are removed to use `std::ffi`, | ||
| // then this becomes unneeded (although it won't break). | ||
| MapInput::Alias(a) => (a.ident(), cdecl::named(a.ident(), Constness::Mut)), | ||
| MapInput::Struct(s) => (s.ident(), cdecl::named(s.ident(), Constness::Mut)), | ||
| MapInput::Union(u) => (u.ident(), cdecl::named(u.ident(), Constness::Mut)), |
There was a problem hiding this comment.
What is the FIXME here? This part doesn't have to do with primitives.
ctest-next/src/translator.rs
Outdated
| // This is done to deal with things like 3usize. | ||
| syn::Expr::Index(i) => { | ||
| let base = translate_expr(&i.expr); | ||
| let index = translate_expr(&i.index); | ||
| format!("{base}[{index}]") | ||
| } | ||
| syn::Expr::Lit(l) => match &l.lit { | ||
| syn::Lit::Int(i) => i.base10_digits().to_string(), | ||
| _ => l.to_token_stream().to_string(), | ||
| }, |
There was a problem hiding this comment.
Is that comment above the right block? For the integer parts, you can turn e.g. 3usize into ((size_t)(3)) to keep the type around.
Could you make sure this is tested in ctest-next/src/tests.rs?
mbyx
force-pushed
the
ctest-next-bug-fix
branch
from
a0c27ab to
0bf97d4
Compare
tgross35
left a comment
tgross35
left a comment
There was a problem hiding this comment.
Looks great, thank you for including a test for the indexing bit!
mbyx
force-pushed
the
ctest-next-bug-fix
branch
from
0bf97d4 to
2cc9412
Compare
tgross35
force-pushed
the
ctest-next-bug-fix
branch
from
2cc9412 to
d325012
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
mbyx
force-pushed
the
ctest-next-bug-fix
branch
from
d325012 to
6b304ae
Compare
tgross35
added
stable-unneeded
tgross35
added
stable-applied
3 participants
Description
Prepares for porting
libc-testplatforms to usectest-nextby fixing numerous bugs. #4600 and #4610 both depend on this PR, so this should be merged first.Fixes:
ctest-nextin the future, which is a edition 2024 crate.translate_exprto support translating[ty; 3usize]properly.ctest-nextbacked platforms.ffi_itemsto theTranslateHelpersincetemplate.rsdoes the test filtering for consistency.struct const StructName*, and sometimes the struct label would not be applied at all.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