From d14d8d5733ba0e6a3f0cb39d354d719c933afc9d Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Tue, 21 May 2024 11:19:37 -0500 Subject: [PATCH] Remove wcstringutil::split_string() It is short and simple enough to write yourself if you need it and it encourages bad behavior by a) always returning owned strings, b) always allocating them in a vector. If/where possible, it is better to a) use &wstr, b) use an iterator. In rust, it's an anti-pattern to unnecessarily abstract over allocating operations. Some of the call sites even called split_string(..).into_iter(). --- src/builtins/argparse.rs | 5 ++--- src/builtins/string/length.rs | 12 ++++++++---- src/builtins/string/shorten.rs | 10 +++++----- src/env_universal_common.rs | 4 ++-- src/wcstringutil.rs | 5 ----- src/wutil/mod.rs | 6 +++--- 6 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/builtins/argparse.rs b/src/builtins/argparse.rs index e3b8030ce..a9503aa06 100644 --- a/src/builtins/argparse.rs +++ b/src/builtins/argparse.rs @@ -4,7 +4,6 @@ use crate::env::{EnvMode, EnvStack}; use crate::exec::exec_subshell; -use crate::wcstringutil::split_string; use crate::wutil::fish_iswalnum; const VAR_NAME_PREFIX: &wstr = L!("_flag_"); @@ -158,7 +157,7 @@ fn check_for_mutually_exclusive_flags( // information to parse the values associated with any `--exclusive` flags. fn parse_exclusive_args(opts: &mut ArgParseCmdOpts, streams: &mut IoStreams) -> Option { for raw_xflags in &opts.raw_exclusive_flags { - let xflags = split_string(raw_xflags, ','); + let xflags: Vec<_> = raw_xflags.split(',').collect(); if xflags.len() < 2 { streams.err.append(wgettext_fmt!( "%ls: exclusive flag string '%ls' is not valid\n", @@ -169,7 +168,7 @@ fn parse_exclusive_args(opts: &mut ArgParseCmdOpts, streams: &mut IoStreams) -> } let exclusive_set: &mut Vec = &mut vec![]; - for flag in &xflags { + for flag in xflags { if flag.char_count() == 1 && opts.options.contains_key(&flag.char_at(0)) { let short = flag.char_at(0); // It's a short flag. diff --git a/src/builtins/string/length.rs b/src/builtins/string/length.rs index 997854b2d..14da55951 100644 --- a/src/builtins/string/length.rs +++ b/src/builtins/string/length.rs @@ -1,7 +1,5 @@ use super::*; -use crate::wcstringutil::split_string; - #[derive(Default)] pub struct Length { quiet: bool, @@ -36,11 +34,17 @@ fn handle( for (arg, _) in arguments(args, optind, streams) { if self.visible { // Visible length only makes sense line-wise. - for line in split_string(&arg, '\n') { + for line in { + let val: &wstr = &arg; + val.split('\n') + } { let mut max = 0; // Carriage-return returns us to the beginning. The longest substring without // carriage-return determines the overall width. - for reset in split_string(&line, '\r') { + for reset in { + let val = &line; + val.split('\r') + } { let n = width_without_escapes(&reset, 0); max = usize::max(max, n); } diff --git a/src/builtins/string/shorten.rs b/src/builtins/string/shorten.rs index e74b48e55..48c7b7d77 100644 --- a/src/builtins/string/shorten.rs +++ b/src/builtins/string/shorten.rs @@ -1,6 +1,5 @@ use super::*; use crate::common::get_ellipsis_str; -use crate::wcstringutil::split_string; pub struct Shorten<'args> { ellipsis: &'args wstr, @@ -92,13 +91,14 @@ fn handle( // Visible width only makes sense line-wise. // So either we have no-newlines (which means we shorten on the first newline), // or we handle the lines separately. - let mut splits = split_string(&arg, '\n').into_iter(); - if self.no_newline && splits.len() > 1 { + let mut splits = arg.split('\n').peekable(); + if self.no_newline && splits.peek().is_some() { let mut s = match self.shorten_from { Direction::Right => splits.next(), Direction::Left => splits.last(), } - .unwrap(); + .unwrap() + .to_owned(); s.push_utfstr(&self.ellipsis); let width = width_without_escapes(&s, 0); @@ -112,7 +112,7 @@ fn handle( if width > 0 && width < min_width { min_width = width; } - inputs.push(s); + inputs.push(s.into()); } } } diff --git a/src/env_universal_common.rs b/src/env_universal_common.rs index 6d8aec152..ac4b97053 100644 --- a/src/env_universal_common.rs +++ b/src/env_universal_common.rs @@ -11,7 +11,7 @@ use crate::path::path_get_config; use crate::path::{path_get_config_remoteness, DirRemoteness}; use crate::wchar::{decode_byte_from_char, prelude::*}; -use crate::wcstringutil::{join_strings, split_string, string_suffixes_string, LineIterator}; +use crate::wcstringutil::{join_strings, string_suffixes_string, LineIterator}; use crate::wutil::{ file_id_for_fd, file_id_for_path, file_id_for_path_narrow, wdirname, wrealpath, wrename, wstat, wunlink, FileId, INVALID_FILE_ID, @@ -995,7 +995,7 @@ fn decode_serialized(val: &wstr) -> Vec { if val == ENV_NULL { return vec![]; } - split_string(val, UVAR_ARRAY_SEP) + val.split(UVAR_ARRAY_SEP).map(|v| v.to_owned()).collect() } /// Decode a a list into a serialized universal variable value. diff --git a/src/wcstringutil.rs b/src/wcstringutil.rs index 08b6e957e..39c080fc4 100644 --- a/src/wcstringutil.rs +++ b/src/wcstringutil.rs @@ -325,11 +325,6 @@ fn wcs2string_bad_char(c: char) { ); } -/// Split a string by a separator character. -pub fn split_string(val: &wstr, sep: char) -> Vec { - val.split(sep).map(wstr::to_owned).collect() -} - /// Split a string by runs of any of the separator characters provided in `seps`. /// Note the delimiters are the characters in `seps`, not `seps` itself. /// `seps` may contain the NUL character. diff --git a/src/wutil/mod.rs b/src/wutil/mod.rs index 64cc986e0..52cdc8de7 100644 --- a/src/wutil/mod.rs +++ b/src/wutil/mod.rs @@ -17,7 +17,7 @@ use crate::flog::FLOGF; use crate::wchar::{wstr, WString, L}; use crate::wchar_ext::WExt; -use crate::wcstringutil::{join_strings, split_string, wcs2string_callback}; +use crate::wcstringutil::{join_strings, wcs2string_callback}; use errno::errno; pub use gettext::{wgettext, wgettext_fmt, wgettext_maybe_fmt, wgettext_str}; pub use printf::sprintf; @@ -291,8 +291,8 @@ pub fn path_normalize_for_cd(wd: &wstr, path: &wstr) -> WString { } // Split our strings by the sep. - let mut wd_comps = split_string(wd, sep); - let path_comps = split_string(path, sep); + let mut wd_comps = wd.split(sep).collect::>(); + let path_comps = path.split(sep).collect::>(); // Remove empty segments from wd_comps. // In particular this removes the leading and trailing empties.