From ea220774965fffa9fca2ddf0f2852749ecf26279 Mon Sep 17 00:00:00 2001 From: Christiaan Biesterbosch Date: Wed, 18 Sep 2024 15:41:30 +0200 Subject: [PATCH 1/2] Rewrite `compare_src_locs` implementation to have a total order This implementation is simplified compared to the previous one. It is also almost twice as slow in the exhaustive test (15 vs 25 seconds) in https://github.com/immunant/c2rust/issues/1126#issuecomment-2354876003 However, in real sort usage the impact should be significantly less. Fixes https://github.com/immunant/c2rust/issues/1126 --- c2rust-transpile/src/c_ast/mod.rs | 33 ++++++++++++++++++------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/c2rust-transpile/src/c_ast/mod.rs b/c2rust-transpile/src/c_ast/mod.rs index 182b795812..66f346aff9 100644 --- a/c2rust-transpile/src/c_ast/mod.rs +++ b/c2rust-transpile/src/c_ast/mod.rs @@ -206,30 +206,35 @@ impl TypedAstContext { self.files[id].path.as_deref() } + /// Compare two [`SrcLoc`]s based on their import path pub fn compare_src_locs(&self, a: &SrcLoc, b: &SrcLoc) -> Ordering { - /// Compare `self` with `other`, without regard to file id - fn cmp_pos(a: &SrcLoc, b: &SrcLoc) -> Ordering { - (a.line, a.column).cmp(&(b.line, b.column)) - } - let path_a = self.include_map[self.file_map[a.fileid as usize]].clone(); - let path_b = self.include_map[self.file_map[b.fileid as usize]].clone(); + use Ordering::*; + let path_a = &self.include_map[self.file_map[a.fileid as usize]]; + let path_b = &self.include_map[self.file_map[b.fileid as usize]]; + + // Find the first include that does not match between the two for (include_a, include_b) in path_a.iter().zip(path_b.iter()) { - if include_a.fileid != include_b.fileid { - return cmp_pos(include_a, include_b); + let order = include_a.cmp(include_b); + if order != Equal { + return order; } } - use Ordering::*; + + // Either all parent includes are the same, or the include paths are of different lengths + // because .zip() stops when one of the iterators is empty. match path_a.len().cmp(&path_b.len()) { Less => { - // compare the place b was included in a'a file with a + // a has the shorter path, which means b was included in a's file + // so extract that include and compare the position to a let b = path_b.get(path_a.len()).unwrap(); - cmp_pos(a, b) + a.cmp(b) } - Equal => cmp_pos(a, b), + Equal => a.cmp(b), // a and b have the same include path and are thus in the same file Greater => { - // compare the place a was included in b's file with b + // b has the shorter path, which means a was included in b's file + // so extract that include and compare the position to b let a = path_a.get(path_b.len()).unwrap(); - cmp_pos(a, b) + a.cmp(b) } } } From ea15739f537e5b425efc9342234ef1c47bb5d653 Mon Sep 17 00:00:00 2001 From: Christiaan Biesterbosch Date: Mon, 23 Sep 2024 17:12:23 +0200 Subject: [PATCH 2/2] Add test for checking the total order of `fn compare_src_locs` Also included some code suggestions by Khyber. Co-authored-by: Khyber Sen --- c2rust-transpile/src/c_ast/mod.rs | 265 ++++++++++++++++++++++++++++-- 1 file changed, 253 insertions(+), 12 deletions(-) diff --git a/c2rust-transpile/src/c_ast/mod.rs b/c2rust-transpile/src/c_ast/mod.rs index 66f346aff9..9506eac705 100644 --- a/c2rust-transpile/src/c_ast/mod.rs +++ b/c2rust-transpile/src/c_ast/mod.rs @@ -213,30 +213,30 @@ impl TypedAstContext { let path_b = &self.include_map[self.file_map[b.fileid as usize]]; // Find the first include that does not match between the two - for (include_a, include_b) in path_a.iter().zip(path_b.iter()) { - let order = include_a.cmp(include_b); - if order != Equal { - return order; - } + let common_len = path_a.len().min(path_b.len()); + let order = path_a[..common_len].cmp(&path_b[..common_len]); + if order != Equal { + return order; } // Either all parent includes are the same, or the include paths are of different lengths // because .zip() stops when one of the iterators is empty. - match path_a.len().cmp(&path_b.len()) { + let (a, b) = match path_a.len().cmp(&path_b.len()) { Less => { // a has the shorter path, which means b was included in a's file // so extract that include and compare the position to a - let b = path_b.get(path_a.len()).unwrap(); - a.cmp(b) + let b = &path_b[path_a.len()]; + (a, b) } - Equal => a.cmp(b), // a and b have the same include path and are thus in the same file + Equal => (a, b), // a and b have the same include path and are thus in the same file Greater => { // b has the shorter path, which means a was included in b's file // so extract that include and compare the position to b - let a = path_a.get(path_b.len()).unwrap(); - a.cmp(b) + let a = &path_a[path_b.len()]; + (a, b) } - } + }; + a.cmp(b) } pub fn get_file_include_line_number(&self, file: FileId) -> Option { @@ -1870,3 +1870,244 @@ impl CTypeKind { Some(ty) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_compare_src_locs_ord() { + let ctx = TypedAstContext { + c_types: Default::default(), + c_exprs: Default::default(), + c_stmts: Default::default(), + c_decls: Default::default(), + c_decls_top: vec![], + c_main: None, + parents: Default::default(), + files: vec![], + file_map: vec![0, 1, 2, 3, 4, 5, 4, 5], + include_map: vec![ + vec![], + vec![], + vec![SrcLoc { + fileid: 2, + line: 6, + column: 10, + }], + vec![], + vec![], + vec![SrcLoc { + fileid: 5, + line: 6, + column: 10, + }], + ], + label_names: Default::default(), + macro_invocations: Default::default(), + macro_expansions: Default::default(), + macro_expansion_text: Default::default(), + comments: vec![], + prenamed_decls: Default::default(), + va_list_kind: BuiltinVaListKind::CharPtrBuiltinVaList, + target: "".to_string(), + }; + let locs = &mut [ + SrcLoc { + fileid: 5, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 1, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 1, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 1, + line: 10, + column: 1, + }, + SrcLoc { + fileid: 1, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 2, + line: 4, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 3, + column: 7, + }, + SrcLoc { + fileid: 5, + line: 3, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 3, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 7, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 7, + }, + SrcLoc { + fileid: 5, + line: 7, + column: 7, + }, + SrcLoc { + fileid: 5, + line: 7, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 7, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 9, + column: 7, + }, + SrcLoc { + fileid: 5, + line: 9, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 9, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 7, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 5, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 8, + column: 3, + }, + SrcLoc { + fileid: 5, + line: 8, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 7, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 0, + column: 4, + }, + SrcLoc { + fileid: 5, + line: 0, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 2, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 98, + column: 3, + }, + SrcLoc { + fileid: 5, + line: 98, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 202, + column: 1, + }, + SrcLoc { + fileid: 5, + line: 202, + column: 1, + }, + SrcLoc { + fileid: 7, + line: 1, + column: 1, + }, + SrcLoc { + fileid: 7, + line: 3, + column: 1, + }, + ]; + + let n = locs.len(); + for i in 0..n { + let a = locs[i]; + for j in 0..n { + let b = locs[j]; + for k in 0..n { + let c = locs[k]; + let ab = ctx.compare_src_locs(&a, &b); + let bc = ctx.compare_src_locs(&b, &c); + let ac = ctx.compare_src_locs(&a, &c); + if ab == bc { + assert_eq!(ab, ac, "Total order (transitivity) has been violated"); + } + } + } + } + + // This should not panic + locs.sort_unstable_by(|a, b| ctx.compare_src_locs(a, b)); + } +}