-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Rewrite intrinsic-unreachable
, sepcomp-cci-copies
, sepcomp-inlining
and sepcomp-separate
run-make
tests to rmake.rs
#126427
Conversation
The run-make-support library was changed cc @jieyouxu This PR modifies cc @jieyouxu These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
954fc4f
to
bf298cb
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
paths.filter_map(|entry| entry.ok()).filter(|path| path.is_file()).for_each(|path| { | ||
let file = fs_wrapper::open_file(path); | ||
let reader = io::BufReader::new(file); | ||
reader | ||
.lines() | ||
.filter_map(|line| line.ok()) | ||
.filter(|line| re.is_match(line)) | ||
.for_each(|_| match_count += 1); | ||
}); | ||
match_count |
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.
I have a feeling this can be streamlined using the .count()
method to remove the need to manually increment, but I don't know what the most idiomatic style would be
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.
paths.filter_map(|entry| entry.ok())
.filter(|entry| entry.as_path().is_file())
.filter_map(|path| fs::File::open(&path).ok())
.map(|file| io::BufReader::new(file))
.flat_map(|reader| reader.lines().filter_map(|entry| entry.ok()))
.filter(|line| re.is_match(line))
.count()
Good suggestion, after some tinkering I got this to work. It feels a little bulky, though.
intrinsic-unreachable
, sepcomp-separate
, sepcomp-inlining
and sepcomp-separate
run-make
tests to rmake.rsintrinsic-unreachable
, sepcomp-cci-copies
, sepcomp-inlining
and sepcomp-separate
run-make
tests to rmake.rs
This comment has been minimized.
This comment has been minimized.
/// Inside a glob pattern of files (paths), read their contents and count the | ||
/// number of regex matches with a given expression (re). | ||
#[track_caller] | ||
pub fn count_regex_matches_in_file_glob(re: &str, paths: &str) -> usize { | ||
let re = regex::Regex::new(re).expect(format!("Regex expression {re} is not valid.").as_str()); | ||
let paths = glob::glob(paths).expect(format!("Glob expression {paths} is not valid.").as_str()); | ||
use io::BufRead; | ||
paths | ||
.filter_map(|entry| entry.ok()) | ||
.filter(|entry| entry.as_path().is_file()) | ||
.filter_map(|path| fs::File::open(&path).ok()) | ||
.map(|file| io::BufReader::new(file)) | ||
.flat_map(|reader| reader.lines().filter_map(|entry| entry.ok())) | ||
.filter(|line| re.is_match(line)) | ||
.count() | ||
} |
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.
Discussion: a globbing API makes me very uneasy, I would strongly prefer if this was a walkdir-style API (and specialized helpers built on such an API, see src/tools/tidy
's usage of walkdir-style APIs) instead, because it's not immediately clear if this is recursive, deals with "hidden" files, handles non-UTF8, among other filesystem / glob-related possible failstates and gotchas.
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.
I have tried my hand at a reworked version of this that does not use glob - see the latest commit. Tell me what you think!
fn main() { | ||
rustc().input("cci_lib.rs").run(); | ||
rustc().input("foo.rs").emit("llvm-ir").codegen_units(6).arg("-Zinline-in-all-cgus").run(); | ||
assert_eq!(count_regex_matches_in_file_glob(r#"define\ .*cci_fn"#, "foo.*.ll"), 2); |
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.
Discussion: cf. the support library comment, I feel like this helper API makes it almost too opaque for the test reader of what is being tested.
@rustbot author |
cbf8157
to
dc989e7
Compare
This comment has been minimized.
This comment has been minimized.
Note to myself: revisit this PR @rustbot review |
☔ The latest upstream changes (presumably #126607) made this pull request unmergeable. Please resolve the merge conflicts. |
I think we should just avoid using @rustbot author |
Notes for myself:
|
4bbe6b9
to
64a7343
Compare
@rustbot review |
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.
Tests look good, r=me after hoisting the helper into the support lib.
fn count_regex_matches_in_files_with_extension(re: ®ex::Regex, ext: &str) -> usize { | ||
let fetched_files = shallow_find_files(cwd(), |path| has_extension(path, ext)); | ||
|
||
let mut count = 0; | ||
for file in fetched_files { | ||
let content = fs_wrapper::read_to_string(file); | ||
count += content.lines().filter(|line| re.is_match(&line)).count(); | ||
} | ||
|
||
count | ||
} |
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.
Suggestion: can you hoist this into the support library and add some docs for what it does? I know I may have said to keep it localized in the test files previously, sorry!
@rustbot author |
8e89c65
to
1c69496
Compare
…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#126427 (Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs) - rust-lang#127237 (Improve code of `run-make/llvm-ident`) - rust-lang#127325 (Migrate `target-cpu-native`, `target-specs` and `target-without-atomic-cas` `run-make` tests to rmake) - rust-lang#127482 (Infer async closure signature from (old-style) two-part `Fn` + `Future` bounds) Failed merges: - rust-lang#127357 (Remove `StructuredDiag`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#126427 - Oneirical:oktobertest, r=jieyouxu Rewrite `intrinsic-unreachable`, `sepcomp-cci-copies`, `sepcomp-inlining` and `sepcomp-separate` `run-make` tests to rmake.rs Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).
Part of #121876 and the associated Google Summer of Code project.