unexpand: add support for extended tab stop syntax (+N and /N) #9265
Conversation
should fix tests/misc/unexpand.pl
CodSpeed Performance ReportMerging #9265 will degrade performances by 16.81%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
GNU testsuite comparison: |
| if num == 0 { | ||
| return Err(ParseError::TabSizeCannotBeZero); | ||
| } |
There was a problem hiding this comment.
I would handle this case in its own arm: it makes the match more compact and allows you to get rid of the return.
| fn parse_increment_syntax(word: &str, nums: &[usize]) -> Result<usize, ParseError> { | ||
| if nums.is_empty() { | ||
| return Err(ParseError::InvalidCharacter("+".to_string())); | ||
| } |
There was a problem hiding this comment.
I would remove the nums param, its values are never used. And the check for emptiness should probably be done where this function is called.
| fn parse_extend_syntax(word: &str, nums: &[usize]) -> Result<usize, ParseError> { | ||
| if nums.is_empty() { | ||
| return Err(ParseError::InvalidCharacter("/".to_string())); | ||
| } | ||
| match word.parse::<usize>() { | ||
| Ok(num) => { | ||
| if num == 0 { | ||
| return Err(ParseError::TabSizeCannotBeZero); | ||
| } |
There was a problem hiding this comment.
My two previous comments about parse_increment_syntax also apply to this function.
There was a problem hiding this comment.
With nums removed, you can also remove parse_increment_syntax or parse_extend_syntax because both functions do the same thing. And rename the remaining function.
| } | ||
| } | ||
|
|
||
| fn tabstops_parse(s: &str) -> Result<TabConfig, ParseError> { |
There was a problem hiding this comment.
I would name this function parse_tabstops to be consistent with the other function names that start with parse.
| match word.parse::<usize>() { | ||
| Ok(num) => nums.push(num), | ||
| Err(e) => { | ||
| return match e.kind() { | ||
| IntErrorKind::PosOverflow => Err(ParseError::TabSizeTooLarge), | ||
| _ => Err(ParseError::InvalidCharacter( | ||
| word.trim_start_matches(char::is_numeric).to_string(), | ||
| )), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
This functionality already exists in the function you get when applying the refactorings I mentioned previously. The result of this function you can then push to nums.
| if nums.contains(&0) { | ||
| return Err(ParseError::TabSizeCannotBeZero); | ||
| } |
There was a problem hiding this comment.
With my previous suggestions this snippet is no longer necessary as the condition can't become true.
| if nums.is_empty() { | ||
| return Err(ParseError::InvalidCharacter("+".to_string())); | ||
| } | ||
| let last = *nums.last().unwrap(); | ||
| nums.push(last + inc); | ||
| } else if extend_size.is_some() && nums.is_empty() { |
There was a problem hiding this comment.
I think nums.is_empty() is always false in those two conditions and thus the returns are unreachable.
| } else if tab_config.increment_size.is_some() { | ||
| // +N: increment handled in tabstops_parse, so we should have the tab | ||
| None |
There was a problem hiding this comment.
I would remove the condition: it is not relevant as the if and else cases both return the same value.
| } else if tab_config.increment_size.is_some() { | |
| // +N: increment handled in tabstops_parse, so we should have the tab | |
| None |
| match word.parse::<usize>() { | ||
| Ok(num) => { | ||
| if num == 0 { | ||
| return Err(ParseError::TabSizeCannotBeZero); |
There was a problem hiding this comment.
\0 and +0 are accepted by GNU unexpand actually
3 participants
should fix tests/misc/unexpand.pl