Rollup merge of #105829 - the8472:tidy-style, r=jyn514
Speed up tidy This can be reviewed commit by commit since they contain separate optimizations. ``` # master $ taskset -c 0-5 hyperfine './x test tidy' Benchmark #1: ./x test tidy Time (mean ± σ): 4.857 s ± 0.064 s [User: 12.967 s, System: 2.014 s] Range (min … max): 4.779 s … 4.997 s 10 runs # PR $ taskset -c 0-5 hyperfine './x test tidy' Benchmark #1: ./x test tidy Time (mean ± σ): 3.672 s ± 0.035 s [User: 10.524 s, System: 2.029 s] Range (min … max): 3.610 s … 3.725 s 10 runs ```
This commit is contained in:
Matthias Krüger
GitHub
commit
cf08eda0b3
@ -8,7 +8,7 @@
|
||||
use std::process::{Command, Stdio};
|
||||
use std::sync::mpsc::SyncSender;
|
||||
|
||||
fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut() {
|
||||
fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl FnMut(bool) -> bool {
|
||||
let mut cmd = Command::new(&rustfmt);
|
||||
// avoid the submodule config paths from coming into play,
|
||||
// we only allow a single global config for the workspace for now
|
||||
@ -23,7 +23,13 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
|
||||
let cmd_debug = format!("{:?}", cmd);
|
||||
let mut cmd = cmd.spawn().expect("running rustfmt");
|
||||
// poor man's async: return a closure that'll wait for rustfmt's completion
|
||||
move || {
|
||||
move |block: bool| -> bool {
|
||||
if !block {
|
||||
match cmd.try_wait() {
|
||||
Ok(Some(_)) => {}
|
||||
_ => return false,
|
||||
}
|
||||
}
|
||||
let status = cmd.wait().unwrap();
|
||||
if !status.success() {
|
||||
eprintln!(
|
||||
@ -34,6 +40,7 @@ fn rustfmt(src: &Path, rustfmt: &Path, paths: &[PathBuf], check: bool) -> impl F
|
||||
);
|
||||
crate::detail_exit(1);
|
||||
}
|
||||
true
|
||||
}
|
||||
}
|
||||
|
||||
@ -146,15 +153,23 @@ pub fn format(build: &Builder<'_>, check: bool, paths: &[PathBuf]) {
|
||||
let child = rustfmt(&src, &rustfmt_path, paths.as_slice(), check);
|
||||
children.push_back(child);
|
||||
|
||||
// poll completion before waiting
|
||||
for i in (0..children.len()).rev() {
|
||||
if children[i](false) {
|
||||
children.swap_remove_back(i);
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
if children.len() >= max_processes {
|
||||
// await oldest child
|
||||
children.pop_front().unwrap()();
|
||||
children.pop_front().unwrap()(true);
|
||||
}
|
||||
}
|
||||
|
||||
// await remaining children
|
||||
for mut child in children {
|
||||
child();
|
||||
child(true);
|
||||
}
|
||||
});
|
||||
|
||||
|
||||
@ -35,15 +35,26 @@ fn main() {
|
||||
|
||||
let bad = std::sync::Arc::new(AtomicBool::new(false));
|
||||
|
||||
let drain_handles = |handles: &mut VecDeque<ScopedJoinHandle<'_, ()>>| {
|
||||
// poll all threads for completion before awaiting the oldest one
|
||||
for i in (0..handles.len()).rev() {
|
||||
if handles[i].is_finished() {
|
||||
handles.swap_remove_back(i).unwrap().join().unwrap();
|
||||
}
|
||||
}
|
||||
|
||||
while handles.len() >= concurrency.get() {
|
||||
handles.pop_front().unwrap().join().unwrap();
|
||||
}
|
||||
};
|
||||
|
||||
scope(|s| {
|
||||
let mut handles: VecDeque<ScopedJoinHandle<'_, ()>> =
|
||||
VecDeque::with_capacity(concurrency.get());
|
||||
|
||||
macro_rules! check {
|
||||
($p:ident $(, $args:expr)* ) => {
|
||||
while handles.len() >= concurrency.get() {
|
||||
handles.pop_front().unwrap().join().unwrap();
|
||||
}
|
||||
drain_handles(&mut handles);
|
||||
|
||||
let handle = s.spawn(|| {
|
||||
let mut flag = false;
|
||||
@ -97,9 +108,8 @@ macro_rules! check {
|
||||
check!(alphabetical, &library_path);
|
||||
|
||||
let collected = {
|
||||
while handles.len() >= concurrency.get() {
|
||||
handles.pop_front().unwrap().join().unwrap();
|
||||
}
|
||||
drain_handles(&mut handles);
|
||||
|
||||
let mut flag = false;
|
||||
let r = features::check(&src_path, &compiler_path, &library_path, &mut flag, verbose);
|
||||
if flag {
|
||||
|
||||
@ -17,7 +17,7 @@
|
||||
//! `// ignore-tidy-CHECK-NAME`.
|
||||
|
||||
use crate::walk::{filter_dirs, walk};
|
||||
use regex::Regex;
|
||||
use regex::{Regex, RegexSet};
|
||||
use std::path::Path;
|
||||
|
||||
/// Error code markdown is restricted to 80 columns because they can be
|
||||
@ -225,6 +225,7 @@ fn skip(path: &Path) -> bool {
|
||||
.chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:x}", v)))
|
||||
.chain(PROBLEMATIC_CONSTS.iter().map(|v| format!("{:X}", v)))
|
||||
.collect();
|
||||
let problematic_regex = RegexSet::new(problematic_consts_strings.as_slice()).unwrap();
|
||||
walk(path, &mut skip, &mut |entry, contents| {
|
||||
let file = entry.path();
|
||||
let filename = file.file_name().unwrap().to_string_lossy();
|
||||
@ -281,7 +282,27 @@ fn skip(path: &Path) -> bool {
|
||||
let mut trailing_new_lines = 0;
|
||||
let mut lines = 0;
|
||||
let mut last_safety_comment = false;
|
||||
let is_test = file.components().any(|c| c.as_os_str() == "tests");
|
||||
// scanning the whole file for multiple needles at once is more efficient than
|
||||
// executing lines times needles separate searches.
|
||||
let any_problematic_line = problematic_regex.is_match(contents);
|
||||
for (i, line) in contents.split('\n').enumerate() {
|
||||
if line.is_empty() {
|
||||
if i == 0 {
|
||||
leading_new_lines = true;
|
||||
}
|
||||
trailing_new_lines += 1;
|
||||
continue;
|
||||
} else {
|
||||
trailing_new_lines = 0;
|
||||
}
|
||||
|
||||
let trimmed = line.trim();
|
||||
|
||||
if !trimmed.starts_with("//") {
|
||||
lines += 1;
|
||||
}
|
||||
|
||||
let mut err = |msg: &str| {
|
||||
tidy_error!(bad, "{}:{}: {}", file.display(), i + 1, msg);
|
||||
};
|
||||
@ -308,28 +329,29 @@ fn skip(path: &Path) -> bool {
|
||||
suppressible_tidy_err!(err, skip_cr, "CR character");
|
||||
}
|
||||
if filename != "style.rs" {
|
||||
if line.contains("TODO") {
|
||||
if trimmed.contains("TODO") {
|
||||
err("TODO is deprecated; use FIXME")
|
||||
}
|
||||
if line.contains("//") && line.contains(" XXX") {
|
||||
if trimmed.contains("//") && trimmed.contains(" XXX") {
|
||||
err("XXX is deprecated; use FIXME")
|
||||
}
|
||||
for s in problematic_consts_strings.iter() {
|
||||
if line.contains(s) {
|
||||
err("Don't use magic numbers that spell things (consider 0x12345678)");
|
||||
if any_problematic_line {
|
||||
for s in problematic_consts_strings.iter() {
|
||||
if trimmed.contains(s) {
|
||||
err("Don't use magic numbers that spell things (consider 0x12345678)");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
let is_test = || file.components().any(|c| c.as_os_str() == "tests");
|
||||
// for now we just check libcore
|
||||
if line.contains("unsafe {") && !line.trim().starts_with("//") && !last_safety_comment {
|
||||
if file.components().any(|c| c.as_os_str() == "core") && !is_test() {
|
||||
if trimmed.contains("unsafe {") && !trimmed.starts_with("//") && !last_safety_comment {
|
||||
if file.components().any(|c| c.as_os_str() == "core") && !is_test {
|
||||
suppressible_tidy_err!(err, skip_undocumented_unsafe, "undocumented unsafe");
|
||||
}
|
||||
}
|
||||
if line.contains("// SAFETY:") {
|
||||
if trimmed.contains("// SAFETY:") {
|
||||
last_safety_comment = true;
|
||||
} else if line.trim().starts_with("//") || line.trim().is_empty() {
|
||||
} else if trimmed.starts_with("//") || trimmed.is_empty() {
|
||||
// keep previous value
|
||||
} else {
|
||||
last_safety_comment = false;
|
||||
@ -337,7 +359,8 @@ fn skip(path: &Path) -> bool {
|
||||
if (line.starts_with("// Copyright")
|
||||
|| line.starts_with("# Copyright")
|
||||
|| line.starts_with("Copyright"))
|
||||
&& (line.contains("Rust Developers") || line.contains("Rust Project Developers"))
|
||||
&& (trimmed.contains("Rust Developers")
|
||||
|| trimmed.contains("Rust Project Developers"))
|
||||
{
|
||||
suppressible_tidy_err!(
|
||||
err,
|
||||
@ -351,18 +374,6 @@ fn skip(path: &Path) -> bool {
|
||||
if filename.ends_with(".cpp") && line.contains("llvm_unreachable") {
|
||||
err(LLVM_UNREACHABLE_INFO);
|
||||
}
|
||||
if line.is_empty() {
|
||||
if i == 0 {
|
||||
leading_new_lines = true;
|
||||
}
|
||||
trailing_new_lines += 1;
|
||||
} else {
|
||||
trailing_new_lines = 0;
|
||||
}
|
||||
|
||||
if !line.trim().starts_with("//") {
|
||||
lines += 1;
|
||||
}
|
||||
}
|
||||
if leading_new_lines {
|
||||
let mut err = |_| {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user