unexpand: add support for extended tab stop syntax (+N and /N) #9265

sylvestre · 2025-11-13T21:19:27Z

should fix tests/misc/unexpand.pl

codspeed-hq · 2025-11-13T21:32:15Z

Merging this PR will degrade performance by 15.47%

Summary

❌ 2 regressed benchmarks
✅ 81 untouched benchmarks
⏩ 94 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
❌	`unexpand_large_file[10]`	529 ms	625.8 ms	-15.47%
❌	`unexpand_many_lines[100000]`	252.3 ms	298.5 ms	-15.47%

_{Comparing sylvestre:unexpand (c4f6dab) with main (20a5c3a)}

94 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

github-actions · 2025-11-13T21:40:33Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/misc/unexpand is no longer failing!

cakebaker · 2025-11-14T08:25:17Z

src/uu/unexpand/src/unexpand.rs

+            if num == 0 {
+                return Err(ParseError::TabSizeCannotBeZero);
+            }


I would handle this case in its own arm: it makes the match more compact and allows you to get rid of the return.

cakebaker · 2025-11-14T08:51:33Z

src/uu/unexpand/src/unexpand.rs

+fn parse_increment_syntax(word: &str, nums: &[usize]) -> Result<usize, ParseError> {
+    if nums.is_empty() {
+        return Err(ParseError::InvalidCharacter("+".to_string()));
+    }


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.

cakebaker · 2025-11-14T08:54:11Z

src/uu/unexpand/src/unexpand.rs

+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);
+            }


My two previous comments about parse_increment_syntax also apply to this function.

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.

cakebaker · 2025-11-14T09:02:28Z

src/uu/unexpand/src/unexpand.rs

+    }
+}
+
+fn tabstops_parse(s: &str) -> Result<TabConfig, ParseError> {


I would name this function parse_tabstops to be consistent with the other function names that start with parse.

cakebaker · 2025-11-14T09:34:05Z

src/uu/unexpand/src/unexpand.rs

+            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(),
+                        )),
+                    };
+                }
            }


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.

cakebaker · 2025-11-14T09:39:03Z

src/uu/unexpand/src/unexpand.rs

    if nums.contains(&0) {
        return Err(ParseError::TabSizeCannotBeZero);
    }


With my previous suggestions this snippet is no longer necessary as the condition can't become true.

cakebaker · 2025-11-14T09:58:51Z

src/uu/unexpand/src/unexpand.rs

+        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() {


I think nums.is_empty() is always false in those two conditions and thus the returns are unreachable.

cakebaker · 2025-11-14T10:08:26Z

src/uu/unexpand/src/unexpand.rs

+                } else if tab_config.increment_size.is_some() {
+                    // +N: increment handled in tabstops_parse, so we should have the tab
+                    None


I would remove the condition: it is not relevant as the if and else cases both return the same value.

Suggested change

} else if tab_config.increment_size.is_some() {

// +N: increment handled in tabstops_parse, so we should have the tab

None

Not sure what used to be here for this comment:

but I would suggest to do:

} else if let Some(inc) = tab_config.increment_size { let last = *tabstops.last().unwrap(); Some(inc - ((col - last) % inc)) }

Because it causes this difference in behaviour:

# GNU (correct): $ printf " " | unexpand -t '3,+6' | xxd 00000000: 0909 0920 2020 2020 # 3 tabs + 5 spaces # uutils (incorrect): $ printf " " | unexpand -t '3,+6' | xxd 00000000: 0909 2020 2020 2020 2020 2020 20 # 2 tabs + 11 spaces

FidelSch · 2025-11-14T22:45:02Z

src/uu/unexpand/src/unexpand.rs

+    match word.parse::<usize>() {
+        Ok(num) => {
+            if num == 0 {
+                return Err(ParseError::TabSizeCannotBeZero);


\0 and +0 are accepted by GNU unexpand actually

should fix tests/misc/unexpand.pl

github-actions · 2025-12-23T23:23:06Z

GNU testsuite comparison:

Congrats! The gnu test tests/misc/unexpand is no longer failing!

ChrisDryden · 2026-01-06T22:42:43Z

src/uu/unexpand/src/unexpand.rs

 impl UError for ParseError {}

-fn tabstops_parse(s: &str) -> Result<Vec<usize>, ParseError> {
+fn parse_tab_num(word: &str) -> Result<usize, ParseError> {


There's a bit of deviation from GNU here:

# GNU (accepts): $ printf " test" | unexpand -t '3,/0' test # uutils (rejects): $ printf " test" | unexpand -t '3,/0' unexpand: tab size cannot be 0

Since on line 40 the tab size has to unconditionally be 0

ChrisDryden · 2026-01-06T22:53:57Z

src/uu/unexpand/src/unexpand.rs

+            if increment_size.is_some() || extend_size.is_some() {
+                return Err(ParseError::InvalidCharacter("+".to_string()));
+            }
+            if nums.is_empty() {


This line can be removed entirely same as the line on 78,

GNU accepts /N and +N alone, treating them as tabs at multiples of N

Example test cases:

$ printf " " | unexpand -t '/9' | xxd # GNU: 00000000: 09 # uutils: unexpand: tab size contains invalid character(s): '/' $ printf " " | unexpand -t '/9' | xxd # GNU: 00000000: 0909 # uutils: unexpand: tab size contains invalid character(s): '/' $ printf " " | unexpand -t '+6' | xxd # GNU: 00000000: 09 # uutils: unexpand: tab size contains invalid character(s): '+' $ printf " " | unexpand -t '+6' | xxd # GNU: 00000000: 0909 # uutils: unexpand: tab size contains invalid character(s): '+'

and

$ printf " " | unexpand -t '3,/0' | xxd # GNU: 00000000: 0909 0920 # uutils: unexpand: tab size cannot be 0 $ printf " " | unexpand -t '3,+0' | xxd # GNU: 00000000: 0909 0920 # uutils: unexpand: tab size cannot be 0 $ printf " " | unexpand -t '3' | xxd # GNU: 00000000: 0909 0920

ChrisDryden · 2026-01-06T22:58:09Z

Any chance with how busy you are with the reviews you'd be interested in me finishing this one up with the bugs? I'm going to add those edge cases that failed to the main gnu test suite.

github-actions · 2026-01-07T21:11:04Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tty/tty-eof (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/misc/unexpand is no longer failing!

sylvestre mentioned this pull request Nov 13, 2025

GNU coreutils 9.9: Test report #9127

Open

cakebaker reviewed Nov 14, 2025

View reviewed changes

cakebaker mentioned this pull request Nov 14, 2025

Unexpand does not support tabstop modifiers #9273

Open

cakebaker linked an issue Nov 14, 2025 that may be closed by this pull request

Unexpand does not support tabstop modifiers #9273

Open

FidelSch reviewed Nov 14, 2025

View reviewed changes

ChrisDryden mentioned this pull request Dec 19, 2025

GNU coreutils 9.9: Detailed Test report 12/19 #9729

Open

unexpand: add support for extended tab stop syntax (+N and /N)

6c0116c

should fix tests/misc/unexpand.pl

sylvestre force-pushed the unexpand branch from 28e9331 to 6c0116c Compare December 23, 2025 22:54

address review comments

4ab95fd

sylvestre requested a review from cakebaker December 26, 2025 23:33

Merge branch 'main' into unexpand

3ada5b8

ChrisDryden reviewed Jan 6, 2026

View reviewed changes

Merge branch 'main' into unexpand

c4f6dab

	} else if tab_config.increment_size.is_some() {
	// +N: increment handled in tabstops_parse, so we should have the tab
	None

Uh oh!

unexpand: add support for extended tab stop syntax (+N and /N) #9265

Are you sure you want to change the base?

unexpand: add support for extended tab stop syntax (+N and /N) #9265

Uh oh!

Conversation

sylvestre commented Nov 13, 2025

Uh oh!

codspeed-hq bot commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will degrade performance by 15.47%

Summary

Performance Changes

Footnotes

Uh oh!

github-actions bot commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisDryden commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codspeed-hq bot commented Nov 13, 2025 •

edited

Loading