numfmt: implement the last changes#11411
Merged
sylvestre merged 17 commits intouutils:mainfrom Apr 4, 2026
Merged
Conversation
Merging this PR will improve performance by 23.64%
Performance Changes
Comparing 
Footnotes
|
xtqqczze
reviewed
Mar 19, 2026
sylvestre
force-pushed
the
numfmt-2
branch
2 times, most recently
from
339ca87 to
524c1b1
Compare
Contributor
Author
|
and it passed! |
sylvestre
force-pushed
the
numfmt-2
branch
2 times, most recently
from
32deeba to
840c848
Compare
Contributor
Author
|
@cakebaker i can split it into several if you want |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
cakebaker
reviewed
Mar 25, 2026
cakebaker
reviewed
Mar 26, 2026
Contributor
@sylvestre yes, please. |
Contributor
Author
|
@cakebaker started: |
|
GNU testsuite comparison: |
cakebaker
reviewed
Apr 1, 2026
src/uu/numfmt/src/format.rs
Outdated
Comment on lines
+83
to
+89
| if valid_part == "." { | ||
| return Some(translate!("numfmt-error-invalid-suffix", "input" => s.quote())); | ||
| } | ||
|
|
||
| if valid_part.ends_with('.') { | ||
| return Some(translate!("numfmt-error-invalid-number", "input" => s.quote())); | ||
| } |
Contributor
There was a problem hiding this comment.
The name valid_part is a bit unlucky as its content can be invalid ;-)
cakebaker
reviewed
Apr 1, 2026
src/uu/numfmt/src/format.rs
Outdated
Comment on lines
+97
to
+114
| let rest = &s[valid_part.len()..]; | ||
| if let Some(after_sep) = rest.strip_prefix(unit_separator) { | ||
| // Check if the next char is a valid suffix letter | ||
| let mut chars = after_sep.chars(); | ||
| let has_suffix = chars | ||
| .next() | ||
| .is_some_and(|c| RawSuffix::try_from(&c).is_ok()); | ||
| if has_suffix { | ||
| let suffix_char_len = 1; | ||
| let with_i = | ||
| chars.next() == Some('i') && [Unit::Auto, Unit::Iec(true)].contains(&unit); | ||
| let suffix_len = if with_i { | ||
| suffix_char_len + 1 | ||
| } else { | ||
| suffix_char_len | ||
| }; | ||
| valid_part.len() + unit_separator.len() + suffix_len | ||
| } else { |
Contributor
There was a problem hiding this comment.
I would move this stuff into its own function to get rid of the nesting and all the else { valid_part.len() }. Something like:
fn some_great_function_name(
s: &str,
valid_part: &str,
unit: Unit,
unit_separator: &str,
) -> Option<usize> {
let after_sep = s.get(valid_part.len()..)?.strip_prefix(unit_separator)?;
let mut chars = after_sep.chars();
let first_char = chars.next()?;
RawSuffix::try_from(&first_char).ok()?;
let is_iec = chars.next() == Some('i') && matches!(unit, Unit::Auto | Unit::Iec(true));
let suffix_len = 1 + usize::from(is_iec);
Some(valid_part.len() + unit_separator.len() + suffix_len)
}And then use it like:
let valid_end =
if !unit_separator.is_empty() && valid_part == find_numeric_beginning(s).unwrap_or("") {
some_great_function_name(s, valid_part, unit, &unit_separator).unwrap_or(valid_part.len())
} else {
valid_part.len()
};
cakebaker
reviewed
Apr 3, 2026
Comment on lines
+128
to
+130
| Some('+' | '-') => { | ||
| Some(translate!("numfmt-error-invalid-number", "input" => s.quote())) | ||
| } |
Contributor
There was a problem hiding this comment.
Is this intentional? GNU numfmt shows an "invalid suffix" error in such cases:
$ numfmt 123-
numfmt: invalid suffix in input: ‘123-’
$ numfmt 123+
numfmt: invalid suffix in input: ‘123+’
|
GNU testsuite comparison: |
Skip GNU numfmt.pl tests not yet supported: unit-sep-22-fail, grp-1,
grp-2, delim-7, field-1, field-range-err-{1-4,6-13}, strtod-6,
strtod-6.1, leading-4, debug-2, devdebug-{1-7,9-11}, help-1,
fmt-err-9, fmt-err-11, fmt-15, ign-err-5, ign-err-m2.2, ign-err-m3.1.
Also unconditionally refresh binary symlinks to avoid stale builds.
…alidation Fixes GNU numfmt.pl tests: grp-1, grp-2, debug-2, fmt-err-9, fmt-err-11.
Buffer formatted output so that --invalid=fail does not duplicate lines on error, and --invalid=abort streams directly for partial output. Fixes GNU numfmt.pl test: field-3. Re-enables the full numfmt.pl test suite by removing the skip list added earlier in the stack.
Build the grouped string in a single forward pass using chunk iteration instead of reversing, inserting separators, then reversing again. This avoids an extra String allocation and collection.
Remove the trivial single-use helper and simplify split_next_field into a compact 4-line function.
The two match arms for length 1 and length 2 had identical RawSuffix::try_from checks. Unify into a single check after a boolean guard.
Flatten the nested block scope by moving the output trait object and format dispatch to the same level, reducing indentation.
Use the existing print_warning helper for the grouping-no-effect message instead of a one-off writeln with util_name(). Use hardcoded "numfmt:" prefix for the failed-to-convert message to match the other debug warnings. Remove now-unused util_name import.
…o format_and_write
GNU test: numfmt.pl fmt-15
GNU test: numfmt.pl unit-sep-22-fail
GNU test: numfmt.pl devdebug-*, help-1, field-1, field-range-err-*
… and apply_grouping
…ix +/- error message
|
GNU testsuite comparison: |
Contributor
Author
|
merging for the next release |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
should fix gnu/tests/numfmt/numfmt.pl