Skip to content

Commit 10d1bec

Browse files
authored
Rework signature flag parsing (nushell#16617)
Simplify the code and try to avoid unnecessary temporary allocations when that is reasonable. - **Remove redundant alloc of flag (variable) name** - **Use arrays for fixed buffers** - **Remove unnecessary intermediate allocation.** - **Replace weird indexing loop with `.iter_mut`** - **Use `.strip_{pre/suf}fix` instead of manual index** There is more work to do: - disallow illegal chars - avoid `String::from_utf8_lossy` Open questions to me: - what is necessary for graceful error tolerance (otherwise I would skip some work.) ## Release notes summary - What our users need to know N/A
1 parent 9029f92 commit 10d1bec

File tree

1 file changed

+17
-38
lines changed

1 file changed

+17
-38
lines changed

crates/nu-parser/src/parser.rs

Lines changed: 17 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -3223,7 +3223,7 @@ pub fn unescape_string(bytes: &[u8], span: Span) -> (Vec<u8>, Option<ParseError>
32233223
let result = char::from_u32(int);
32243224

32253225
if let Some(result) = result {
3226-
let mut buffer = vec![0; 4];
3226+
let mut buffer = [0; 4];
32273227
let result = result.encode_utf8(&mut buffer);
32283228

32293229
for elem in result.bytes() {
@@ -3988,17 +3988,16 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
39883988
if contents.starts_with(b"--") && contents.len() > 2 {
39893989
// Split the long flag from the short flag with the ( character as delimiter.
39903990
// The trailing ) is removed further down.
3991-
let flags: Vec<_> =
3992-
contents.split(|x| x == &b'(').map(|x| x.to_vec()).collect();
3991+
let flags: Vec<_> = contents.split(|x| x == &b'(').collect();
39933992

39943993
let long = String::from_utf8_lossy(&flags[0][2..]).to_string();
39953994
let mut variable_name = flags[0][2..].to_vec();
39963995
// Replace the '-' in a variable name with '_'
3997-
(0..variable_name.len()).for_each(|idx| {
3998-
if variable_name[idx] == b'-' {
3999-
variable_name[idx] = b'_';
3996+
for byte in variable_name.iter_mut() {
3997+
if *byte == b'-' {
3998+
*byte = b'_';
40003999
}
4001-
});
4000+
}
40024001

40034002
if !is_variable(&variable_name) {
40044003
working_set.error(ParseError::Expected(
@@ -4050,28 +4049,6 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
40504049
let short_flag =
40514050
String::from_utf8_lossy(short_flag).to_string();
40524051
let chars: Vec<char> = short_flag.chars().collect();
4053-
let long = String::from_utf8_lossy(&flags[0][2..]).to_string();
4054-
let mut variable_name = flags[0][2..].to_vec();
4055-
4056-
(0..variable_name.len()).for_each(|idx| {
4057-
if variable_name[idx] == b'-' {
4058-
variable_name[idx] = b'_';
4059-
}
4060-
});
4061-
4062-
if !is_variable(&variable_name) {
4063-
working_set.error(ParseError::Expected(
4064-
"valid variable name for this short flag",
4065-
span,
4066-
))
4067-
}
4068-
4069-
let var_id = working_set.add_variable(
4070-
variable_name,
4071-
span,
4072-
Type::Any,
4073-
false,
4074-
);
40754052

40764053
if chars.len() == 1 {
40774054
args.push(Arg::Flag {
@@ -4103,7 +4080,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
41034080
working_set.error(ParseError::Expected("short flag", span));
41044081
}
41054082

4106-
let mut encoded_var_name = vec![0u8; 4];
4083+
let mut encoded_var_name = [0u8; 4];
41074084
let len = chars[0].encode_utf8(&mut encoded_var_name).len();
41084085
let variable_name = encoded_var_name[0..len].to_vec();
41094086

@@ -4134,12 +4111,11 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
41344111
}
41354112
// Short flag alias for long flag, e.g. --b (-a)
41364113
// This is the same as the short flag in --b(-a)
4137-
else if contents.starts_with(b"(-") {
4114+
else if let Some(short_flag) = contents.strip_prefix(b"(-") {
41384115
if matches!(parse_mode, ParseMode::AfterCommaArg) {
41394116
working_set
41404117
.error(ParseError::Expected("parameter or flag", span));
41414118
}
4142-
let short_flag = &contents[2..];
41434119

41444120
let short_flag = if !short_flag.ends_with(b")") {
41454121
working_set.error(ParseError::Expected("short flag", span));
@@ -4173,19 +4149,22 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) ->
41734149
}
41744150
}
41754151
// Positional arg, optional
4176-
else if contents.ends_with(b"?") {
4177-
let contents: Vec<_> = contents[..(contents.len() - 1)].into();
4178-
let name = String::from_utf8_lossy(&contents).to_string();
4152+
else if let Some(optional_param) = contents.strip_suffix(b"?") {
4153+
let name = String::from_utf8_lossy(optional_param).to_string();
41794154

4180-
if !is_variable(&contents) {
4155+
if !is_variable(optional_param) {
41814156
working_set.error(ParseError::Expected(
41824157
"valid variable name for this optional parameter",
41834158
span,
41844159
))
41854160
}
41864161

4187-
let var_id =
4188-
working_set.add_variable(contents, span, Type::Any, false);
4162+
let var_id = working_set.add_variable(
4163+
optional_param.to_vec(),
4164+
span,
4165+
Type::Any,
4166+
false,
4167+
);
41894168

41904169
args.push(Arg::Positional {
41914170
arg: PositionalArg {

0 commit comments

Comments
 (0)