From 47e6e5ee675a78cd8f6f11daa0611f577f6e2f7b Mon Sep 17 00:00:00 2001 From: Zalathar Date: Thu, 7 Dec 2023 11:12:48 +1100 Subject: [PATCH] coverage: Avoid unnecessary macros in unit tests These macros don't provide enough value to justify their complexity, when they can just as easily be functions instead. --- Cargo.lock | 5 -- compiler/rustc_mir_transform/Cargo.toml | 5 -- .../src/coverage/test_macros/Cargo.toml | 7 -- .../src/coverage/test_macros/src/lib.rs | 6 -- .../rustc_mir_transform/src/coverage/tests.rs | 90 +++++++------------ 5 files changed, 33 insertions(+), 80 deletions(-) delete mode 100644 compiler/rustc_mir_transform/src/coverage/test_macros/Cargo.toml delete mode 100644 compiler/rustc_mir_transform/src/coverage/test_macros/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 7f627e2ce6ec..1d7f93c789af 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -795,10 +795,6 @@ dependencies = [ "rustc-demangle", ] -[[package]] -name = "coverage_test_macros" -version = "0.0.0" - [[package]] name = "cpufeatures" version = "0.2.8" @@ -4266,7 +4262,6 @@ dependencies = [ name = "rustc_mir_transform" version = "0.0.0" dependencies = [ - "coverage_test_macros", "either", "itertools", "rustc_arena", diff --git a/compiler/rustc_mir_transform/Cargo.toml b/compiler/rustc_mir_transform/Cargo.toml index c2ca0a6bcb84..9cc083edb44e 100644 --- a/compiler/rustc_mir_transform/Cargo.toml +++ b/compiler/rustc_mir_transform/Cargo.toml @@ -27,8 +27,3 @@ rustc_trait_selection = { path = "../rustc_trait_selection" } smallvec = { version = "1.8.1", features = ["union", "may_dangle"] } tracing = "0.1" # tidy-alphabetical-end - -[dev-dependencies] -# tidy-alphabetical-start -coverage_test_macros = { path = "src/coverage/test_macros" } -# tidy-alphabetical-end diff --git a/compiler/rustc_mir_transform/src/coverage/test_macros/Cargo.toml b/compiler/rustc_mir_transform/src/coverage/test_macros/Cargo.toml deleted file mode 100644 index f753caa91248..000000000000 --- a/compiler/rustc_mir_transform/src/coverage/test_macros/Cargo.toml +++ /dev/null @@ -1,7 +0,0 @@ -[package] -name = "coverage_test_macros" -version = "0.0.0" -edition = "2021" - -[lib] -proc-macro = true diff --git a/compiler/rustc_mir_transform/src/coverage/test_macros/src/lib.rs b/compiler/rustc_mir_transform/src/coverage/test_macros/src/lib.rs deleted file mode 100644 index f41adf667ec5..000000000000 --- a/compiler/rustc_mir_transform/src/coverage/test_macros/src/lib.rs +++ /dev/null @@ -1,6 +0,0 @@ -use proc_macro::TokenStream; - -#[proc_macro] -pub fn let_bcb(item: TokenStream) -> TokenStream { - format!("let bcb{item} = graph::BasicCoverageBlock::from_usize({item});").parse().unwrap() -} diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs index 702fe5f563e5..302cbf05d78b 100644 --- a/compiler/rustc_mir_transform/src/coverage/tests.rs +++ b/compiler/rustc_mir_transform/src/coverage/tests.rs @@ -27,8 +27,6 @@ use super::counters; use super::graph::{self, BasicCoverageBlock}; -use coverage_test_macros::let_bcb; - use itertools::Itertools; use rustc_data_structures::graph::WithNumNodes; use rustc_data_structures::graph::WithSuccessors; @@ -37,6 +35,10 @@ use rustc_middle::mir::*; use rustc_middle::ty; use rustc_span::{self, BytePos, Pos, Span, DUMMY_SP}; +fn bcb(index: u32) -> BasicCoverageBlock { + BasicCoverageBlock::from_u32(index) +} + // All `TEMP_BLOCK` targets should be replaced before calling `to_body() -> mir::Body`. const TEMP_BLOCK: BasicBlock = BasicBlock::MAX; @@ -300,12 +302,15 @@ fn goto_switchint<'a>() -> Body<'a> { mir_body } -macro_rules! assert_successors { - ($basic_coverage_blocks:ident, $i:ident, [$($successor:ident),*]) => { - let mut successors = $basic_coverage_blocks.successors[$i].clone(); - successors.sort_unstable(); - assert_eq!(successors, vec![$($successor),*]); - } +#[track_caller] +fn assert_successors( + basic_coverage_blocks: &graph::CoverageGraph, + bcb: BasicCoverageBlock, + expected_successors: &[BasicCoverageBlock], +) { + let mut successors = basic_coverage_blocks.successors[bcb].clone(); + successors.sort_unstable(); + assert_eq!(successors, expected_successors); } #[test] @@ -334,13 +339,9 @@ fn test_covgraph_goto_switchint() { basic_coverage_blocks.iter_enumerated().collect::>() ); - let_bcb!(0); - let_bcb!(1); - let_bcb!(2); - - assert_successors!(basic_coverage_blocks, bcb0, [bcb1, bcb2]); - assert_successors!(basic_coverage_blocks, bcb1, []); - assert_successors!(basic_coverage_blocks, bcb2, []); + assert_successors(&basic_coverage_blocks, bcb(0), &[bcb(1), bcb(2)]); + assert_successors(&basic_coverage_blocks, bcb(1), &[]); + assert_successors(&basic_coverage_blocks, bcb(2), &[]); } /// Create a mock `Body` with a loop. @@ -418,15 +419,10 @@ fn test_covgraph_switchint_then_loop_else_return() { basic_coverage_blocks.iter_enumerated().collect::>() ); - let_bcb!(0); - let_bcb!(1); - let_bcb!(2); - let_bcb!(3); - - assert_successors!(basic_coverage_blocks, bcb0, [bcb1]); - assert_successors!(basic_coverage_blocks, bcb1, [bcb2, bcb3]); - assert_successors!(basic_coverage_blocks, bcb2, []); - assert_successors!(basic_coverage_blocks, bcb3, [bcb1]); + assert_successors(&basic_coverage_blocks, bcb(0), &[bcb(1)]); + assert_successors(&basic_coverage_blocks, bcb(1), &[bcb(2), bcb(3)]); + assert_successors(&basic_coverage_blocks, bcb(2), &[]); + assert_successors(&basic_coverage_blocks, bcb(3), &[bcb(1)]); } /// Create a mock `Body` with nested loops. @@ -546,21 +542,13 @@ fn test_covgraph_switchint_loop_then_inner_loop_else_break() { basic_coverage_blocks.iter_enumerated().collect::>() ); - let_bcb!(0); - let_bcb!(1); - let_bcb!(2); - let_bcb!(3); - let_bcb!(4); - let_bcb!(5); - let_bcb!(6); - - assert_successors!(basic_coverage_blocks, bcb0, [bcb1]); - assert_successors!(basic_coverage_blocks, bcb1, [bcb2, bcb3]); - assert_successors!(basic_coverage_blocks, bcb2, []); - assert_successors!(basic_coverage_blocks, bcb3, [bcb4]); - assert_successors!(basic_coverage_blocks, bcb4, [bcb5, bcb6]); - assert_successors!(basic_coverage_blocks, bcb5, [bcb1]); - assert_successors!(basic_coverage_blocks, bcb6, [bcb4]); + assert_successors(&basic_coverage_blocks, bcb(0), &[bcb(1)]); + assert_successors(&basic_coverage_blocks, bcb(1), &[bcb(2), bcb(3)]); + assert_successors(&basic_coverage_blocks, bcb(2), &[]); + assert_successors(&basic_coverage_blocks, bcb(3), &[bcb(4)]); + assert_successors(&basic_coverage_blocks, bcb(4), &[bcb(5), bcb(6)]); + assert_successors(&basic_coverage_blocks, bcb(5), &[bcb(1)]); + assert_successors(&basic_coverage_blocks, bcb(6), &[bcb(4)]); } #[test] @@ -595,10 +583,7 @@ fn test_find_loop_backedges_one() { backedges ); - let_bcb!(1); - let_bcb!(3); - - assert_eq!(backedges[bcb1], vec![bcb3]); + assert_eq!(backedges[bcb(1)], &[bcb(3)]); } #[test] @@ -613,13 +598,8 @@ fn test_find_loop_backedges_two() { backedges ); - let_bcb!(1); - let_bcb!(4); - let_bcb!(5); - let_bcb!(6); - - assert_eq!(backedges[bcb1], vec![bcb5]); - assert_eq!(backedges[bcb4], vec![bcb6]); + assert_eq!(backedges[bcb(1)], &[bcb(5)]); + assert_eq!(backedges[bcb(4)], &[bcb(6)]); } #[test] @@ -632,13 +612,11 @@ fn test_traverse_coverage_with_loops() { traversed_in_order.push(bcb); } - let_bcb!(6); - // bcb0 is visited first. Then bcb1 starts the first loop, and all remaining nodes, *except* // bcb6 are inside the first loop. assert_eq!( *traversed_in_order.last().expect("should have elements"), - bcb6, + bcb(6), "bcb6 should not be visited until all nodes inside the first loop have been visited" ); } @@ -656,20 +634,18 @@ fn test_make_bcb_counters() { coverage_counters.make_bcb_counters(&basic_coverage_blocks, bcb_has_coverage_spans); assert_eq!(coverage_counters.num_expressions(), 0); - let_bcb!(1); assert_eq!( 0, // bcb1 has a `Counter` with id = 0 - match coverage_counters.bcb_counter(bcb1).expect("should have a counter") { + match coverage_counters.bcb_counter(bcb(1)).expect("should have a counter") { counters::BcbCounter::Counter { id, .. } => id, _ => panic!("expected a Counter"), } .as_u32() ); - let_bcb!(2); assert_eq!( 1, // bcb2 has a `Counter` with id = 1 - match coverage_counters.bcb_counter(bcb2).expect("should have a counter") { + match coverage_counters.bcb_counter(bcb(2)).expect("should have a counter") { counters::BcbCounter::Counter { id, .. } => id, _ => panic!("expected a Counter"), }