From 1c5c1993dd469f22a89726e6de348b80feed35e9 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Sat, 1 Jul 2023 13:37:44 -0700 Subject: [PATCH] Make wdirname and wbasename go &wstr -> &wstr There is no reason for either of these functions to allocate, so have them not do it. --- fish-rust/src/builtins/status.rs | 8 +-- fish-rust/src/io.rs | 6 +- fish-rust/src/path.rs | 6 +- fish-rust/src/wutil/mod.rs | 61 ++++++++----------- fish-rust/src/wutil/tests.rs | 101 ++++++++++++++----------------- 5 files changed, 83 insertions(+), 99 deletions(-) diff --git a/fish-rust/src/builtins/status.rs b/fish-rust/src/builtins/status.rs index 577817019..7763fa581 100644 --- a/fish-rust/src/builtins/status.rs +++ b/fish-rust/src/builtins/status.rs @@ -457,10 +457,10 @@ pub fn status( STATUS_BASENAME | STATUS_DIRNAME | STATUS_FILENAME => { let res = parser.current_filename_ffi().from_ffi(); let f = match (res.is_empty(), s) { - (false, STATUS_DIRNAME) => wdirname(res), - (false, STATUS_BASENAME) => wbasename(res), - (true, _) => wgettext!("Standard input").to_owned(), - (false, _) => res, + (false, STATUS_DIRNAME) => wdirname(&res), + (false, STATUS_BASENAME) => wbasename(&res), + (true, _) => wgettext!("Standard input"), + (false, _) => &res, }; streams.out.append(wgettext_fmt!("%ls\n", f)); } diff --git a/fish-rust/src/io.rs b/fish-rust/src/io.rs index cfe5a160a..9a6857ad3 100644 --- a/fish-rust/src/io.rs +++ b/fish-rust/src/io.rs @@ -646,10 +646,10 @@ impl IoChain { // or there's a non-directory component, // find the first problematic component for a better message. if [ENOENT, ENOTDIR].contains(&err) { - let mut dname = spec.target.clone(); + let mut dname: &wstr = &spec.target; while !dname.is_empty() { - let next = wdirname(dname.clone()); - if let Some(md) = wstat(&next) { + let next: &wstr = wdirname(dname); + if let Some(md) = wstat(next) { if !md.is_dir() { FLOGF!( warning, diff --git a/fish-rust/src/path.rs b/fish-rust/src/path.rs index 40b13f24c..72e561bef 100644 --- a/fish-rust/src/path.rs +++ b/fish-rust/src/path.rs @@ -312,7 +312,7 @@ fn path_get_path_core>(cmd: &wstr, pathsv: &[S]) -> GetPathResult // Keep the first *interesting* error and path around. // ENOENT isn't interesting because not having a file is the normal case. // Ignore if the parent directory is already inaccessible. - if waccess(&wdirname(proposed_path.clone()), X_OK) == 0 { + if waccess(wdirname(&proposed_path), X_OK) == 0 { best = GetPathResult::new(Some(err), proposed_path); } } @@ -642,8 +642,8 @@ fn create_directory(d: &wstr) -> bool { } None => { if errno().0 == ENOENT { - let dir = wdirname(d); - if create_directory(&dir) && wmkdir(d, 0o700) == 0 { + let dir: &wstr = wdirname(d); + if create_directory(dir) && wmkdir(d, 0o700) == 0 { return true; } } diff --git a/fish-rust/src/wutil/mod.rs b/fish-rust/src/wutil/mod.rs index bffcc8563..f223df86a 100644 --- a/fish-rust/src/wutil/mod.rs +++ b/fish-rust/src/wutil/mod.rs @@ -322,80 +322,71 @@ pub fn path_normalize_for_cd(wd: &wstr, path: &wstr) -> WString { } /// Wide character version of dirname(). -#[widestrs] -pub fn wdirname(path: impl AsRef) -> WString { - let path = path.as_ref(); +pub fn wdirname(mut path: &wstr) -> &wstr { // Do not use system-provided dirname (#7837). // On Mac it's not thread safe, and will error for paths exceeding PATH_MAX. // This follows OpenGroup dirname recipe. // 1: Double-slash stays. - if path == "//"L { - return path.to_owned(); + if path == "//" { + return path; } // 2: All slashes => return slash. - if !path.is_empty() && path.chars().find(|&c| c != '/').is_none() { - return "/"L.to_owned(); + if !path.is_empty() && path.chars().all(|c| c == '/') { + return L!("/"); } // 3: Trim trailing slashes. - let mut path = path.to_owned(); while path.as_char_slice().last() == Some(&'/') { - path.pop(); + path = path.slice_to(path.char_count() - 1); } // 4: No slashes left => return period. - let Some(last_slash) = path.chars().rev().position(|c| c == '/') else { - return "."L.to_owned() + let Some(last_slash) = path.chars().rposition(|c| c == '/') else { + return L!("."); }; // 5: Remove trailing non-slashes. + path = path.slice_to(last_slash + 1); + // 6: Skip as permitted. // 7: Remove trailing slashes again. - path = path - .chars() - .rev() - .skip(last_slash + 1) - .skip_while(|&c| c == '/') - .collect::() - .chars() - .rev() - .collect(); + while path.as_char_slice().last() == Some(&'/') { + path = path.slice_to(path.char_count() - 1); + } // 8: Empty => return slash. if path.is_empty() { - path = "/"L.to_owned(); + return L!("/"); } path } /// Wide character version of basename(). -#[widestrs] -pub fn wbasename(path: impl AsRef) -> WString { - let path = path.as_ref(); +pub fn wbasename(mut path: &wstr) -> &wstr { // This follows OpenGroup basename recipe. // 1: empty => allowed to return ".". This is what system impls do. if path.is_empty() { - return "."L.to_owned(); + return L!("."); } // 2: Skip as permitted. // 3: All slashes => return slash. - if !path.is_empty() && path.chars().find(|&c| c != '/').is_none() { - return "/"L.to_owned(); + if !path.is_empty() && path.chars().all(|c| c == '/') { + return L!("/"); } // 4: Remove trailing slashes. + while path.as_char_slice().last() == Some(&'/') { + path = path.slice_to(path.char_count() - 1); + } + // 5: Remove up to and including last slash. - path.chars() - .rev() - .skip_while(|&c| c == '/') - .take_while(|&c| c != '/') - .collect::() - .chars() - .rev() - .collect() + if let Some(last_slash) = path.chars().rposition(|c| c == '/') { + path = path.slice_from(last_slash + 1); + } + path } /// Wide character version of mkdir. diff --git a/fish-rust/src/wutil/tests.rs b/fish-rust/src/wutil/tests.rs index 44630e04f..548d9de62 100644 --- a/fish-rust/src/wutil/tests.rs +++ b/fish-rust/src/wutil/tests.rs @@ -1,60 +1,53 @@ use super::*; -use libc::PATH_MAX; +use libc; +use widestring_suffix::widestrs; -macro_rules! test_cases_wdirname_wbasename { - ($($name:ident: $test:expr),* $(,)?) => { - $( - #[test] - fn $name() { - let (path, dir, base) = $test; - let actual = wdirname(WString::from(path)); - assert_eq!(actual, WString::from(dir), "Wrong dirname for {:?}", path); - let actual = wbasename(WString::from(path)); - assert_eq!(actual, WString::from(base), "Wrong basename for {:?}", path); - } - )* - }; -} +#[test] +#[widestrs] +fn test_wdirname_wbasename() { + // path, dir, base + struct Test(&'static wstr, &'static wstr, &'static wstr); + const testcases: &[Test] = &[ + Test(""L, "."L, "."L), + Test("foo//"L, "."L, "foo"L), + Test("foo//////"L, "."L, "foo"L), + Test("/////foo"L, "/"L, "foo"L), + Test("//foo/////bar"L, "//foo"L, "bar"L), + Test("foo/////bar"L, "foo"L, "bar"L), + // Examples given in XPG4.2. + Test("/usr/lib"L, "/usr"L, "lib"L), + Test("usr"L, "."L, "usr"L), + Test("/"L, "/"L, "/"L), + Test("."L, "."L, "."L), + Test(".."L, "."L, ".."L), + ]; -/// Helper to return a string whose length greatly exceeds PATH_MAX. -fn overlong_path() -> WString { - let mut longpath = WString::with_capacity((PATH_MAX * 2 + 10) as usize); - while longpath.len() < (PATH_MAX * 2) as usize { + for tc in testcases { + let Test(path, tc_dir, tc_base) = *tc; + let dir = wdirname(&path); + assert_eq!( + dir, tc_dir, + "\npath: {:?}, dir: {:?}, tc.dir: {:?}", + path, dir, tc_dir + ); + + let base = wbasename(&path); + assert_eq!( + base, tc_base, + "\npath: {:?}, base: {:?}, tc.base: {:?}", + path, base, tc_base + ); + } + + // Ensure strings which greatly exceed PATH_MAX still work (#7837). + const PATH_MAX: usize = libc::PATH_MAX as usize; + let mut longpath = WString::new(); + longpath.reserve(PATH_MAX * 2 + 10); + while longpath.char_count() <= PATH_MAX * 2 { longpath.push_str("/overlong"); } - return longpath; -} - -test_cases_wdirname_wbasename! { - wdirname_wbasename_test_1: ("", ".", "."), - wdirname_wbasename_test_2: ("foo//", ".", "foo"), - wdirname_wbasename_test_3: ("foo//////", ".", "foo"), - wdirname_wbasename_test_4: ("/////foo", "/", "foo"), - wdirname_wbasename_test_5: ("/////foo", "/", "foo"), - wdirname_wbasename_test_6: ("//foo/////bar", "//foo", "bar"), - wdirname_wbasename_test_7: ("foo/////bar", "foo", "bar"), - // Examples given in XPG4.2. - wdirname_wbasename_test_8: ("/usr/lib", "/usr", "lib"), - wdirname_wbasename_test_9: ("usr", ".", "usr"), - wdirname_wbasename_test_10: ("/", "/", "/"), - wdirname_wbasename_test_11: (".", ".", "."), - wdirname_wbasename_test_12: ("..", ".", ".."), -} - -// Ensures strings which greatly exceed PATH_MAX still work (#7837). -#[test] -fn test_overlong_wdirname_wbasename() { - let path = overlong_path(); - let dir = { - let mut longpath_dir = path.clone(); - let last_slash = longpath_dir.chars().rev().position(|c| c == '/').unwrap(); - longpath_dir.truncate(longpath_dir.len() - last_slash - 1); - longpath_dir - }; - let base = "overlong"; - - let actual = wdirname(&path); - assert_eq!(actual, dir, "Wrong dirname for {:?}", path); - let actual = wbasename(&path); - assert_eq!(actual, base, "Wrong basename for {:?}", path); + let last_slash = longpath.chars().rposition(|c| c == '/').unwrap(); + let longpath_dir = &longpath[..last_slash]; + assert_eq!(wdirname(&longpath), longpath_dir); + assert_eq!(wbasename(&longpath), "overlong"L); }