Avoid taking unnecessary ownership of intermediates (#12740)

# Description

Judiciously try to avoid allocations/clone by changing the signature of
functions

- **Don't pass str by value unnecessarily if only read**
- **Don't require a vec in `Sandbox::with_files`**
- **Remove unnecessary string clone**
- **Fixup unnecessary borrow**
- **Use `&str` in shape color instead**
- **Vec -> Slice**
- **Elide string clone**
- **Elide `Path` clone**
- **Take &str to elide clone in tests**

# User-Facing Changes
None

# Tests + Formatting
This touches many tests purely in changing from owned to borrowed/static
data
This commit is contained in:
Stefan Holderbach
2024-05-04 02:53:15 +02:00
committed by GitHub
parent e6f473695c
commit 406df7f208
69 changed files with 527 additions and 553 deletions

View File

@ -39,7 +39,7 @@ impl Zone {
Self::Error // Out of range
}
}
fn from_string(s: String) -> Self {
fn from_string(s: &str) -> Self {
match s.to_ascii_lowercase().as_str() {
"utc" | "u" => Self::Utc,
"local" | "l" => Self::Local,
@ -126,7 +126,7 @@ impl Command for SubCommand {
span: zone_offset.span,
}),
None => timezone.as_ref().map(|zone| Spanned {
item: Zone::from_string(zone.item.clone()),
item: Zone::from_string(&zone.item),
span: zone.span,
}),
};

View File

@ -57,7 +57,7 @@ impl Command for ConfigEnv {
let env_vars_str = env_to_strings(engine_state, stack)?;
let nu_config = match engine_state.get_config_path("env-path") {
Some(path) => path.clone(),
Some(path) => path,
None => {
return Err(ShellError::GenericError {
error: "Could not find $nu.env-path".into(),

View File

@ -61,7 +61,7 @@ impl Command for ConfigNu {
let env_vars_str = env_to_strings(engine_state, stack)?;
let nu_config = match engine_state.get_config_path("config-path") {
Some(path) => path.clone(),
Some(path) => path,
None => {
return Err(ShellError::GenericError {
error: "Could not find $nu.config-path".into(),

View File

@ -1,10 +1,10 @@
use crate::ExternalCommand;
use nu_protocol::{OutDest, Span, Spanned};
use std::{collections::HashMap, path::PathBuf};
use std::{collections::HashMap, path::Path};
pub(crate) fn gen_command(
span: Span,
config_path: PathBuf,
config_path: &Path,
item: String,
config_args: Vec<String>,
env_vars_str: HashMap<String, String>,

View File

@ -197,15 +197,15 @@ fn from_document_to_value(d: &roxmltree::Document, info: &ParsingInfo) -> Value
element_to_value(&d.root_element(), info)
}
fn from_xml_string_to_value(s: String, info: &ParsingInfo) -> Result<Value, roxmltree::Error> {
let parsed = roxmltree::Document::parse(&s)?;
fn from_xml_string_to_value(s: &str, info: &ParsingInfo) -> Result<Value, roxmltree::Error> {
let parsed = roxmltree::Document::parse(s)?;
Ok(from_document_to_value(&parsed, info))
}
fn from_xml(input: PipelineData, info: &ParsingInfo) -> Result<PipelineData, ShellError> {
let (concat_string, span, metadata) = input.collect_string_strict(info.span)?;
match from_xml_string_to_value(concat_string, info) {
match from_xml_string_to_value(&concat_string, info) {
Ok(x) => Ok(x.into_pipeline_data_with_metadata(metadata)),
Err(err) => Err(process_xml_parse_error(err, span)),
}
@ -370,7 +370,7 @@ mod tests {
keep_comments: false,
keep_processing_instructions: false,
};
from_xml_string_to_value(xml.to_string(), &info)
from_xml_string_to_value(xml, &info)
}
#[test]

View File

@ -185,14 +185,10 @@ fn convert_yaml_value_to_nu_value(
})
}
pub fn from_yaml_string_to_value(
s: String,
span: Span,
val_span: Span,
) -> Result<Value, ShellError> {
pub fn from_yaml_string_to_value(s: &str, span: Span, val_span: Span) -> Result<Value, ShellError> {
let mut documents = vec![];
for document in serde_yaml::Deserializer::from_str(&s) {
for document in serde_yaml::Deserializer::from_str(s) {
let v: serde_yaml::Value =
serde_yaml::Value::deserialize(document).map_err(|x| ShellError::UnsupportedInput {
msg: format!("Could not load YAML: {x}"),
@ -238,7 +234,7 @@ pub fn get_examples() -> Vec<Example<'static>> {
fn from_yaml(input: PipelineData, head: Span) -> Result<PipelineData, ShellError> {
let (concat_string, span, metadata) = input.collect_string_strict(head)?;
match from_yaml_string_to_value(concat_string, head, span) {
match from_yaml_string_to_value(&concat_string, head, span) {
Ok(x) => Ok(x.into_pipeline_data_with_metadata(metadata)),
Err(other) => Err(other),
}
@ -274,11 +270,7 @@ mod test {
];
let config = Config::default();
for tc in tt {
let actual = from_yaml_string_to_value(
tc.input.to_owned(),
Span::test_data(),
Span::test_data(),
);
let actual = from_yaml_string_to_value(tc.input, Span::test_data(), Span::test_data());
if actual.is_err() {
assert!(
tc.expected.is_err(),
@ -313,11 +305,7 @@ mod test {
// table was non-deterministic. It would take a few executions of the YAML conversion to
// see this ordering difference. This loop should be far more than enough to catch a regression.
for ii in 1..1000 {
let actual = from_yaml_string_to_value(
String::from(test_yaml),
Span::test_data(),
Span::test_data(),
);
let actual = from_yaml_string_to_value(test_yaml, Span::test_data(), Span::test_data());
let expected: Result<Value, ShellError> = Ok(Value::test_list(vec![
Value::test_record(record! {