-
-
Notifications
You must be signed in to change notification settings - Fork 2k
sort: gnu coreutils compatibility (sort float.sh) #9839
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
e9ba6ca
716f6f4
2608a35
9013602
81ec542
3751b81
8da07b8
bf335ca
2366786
38c7316
2f14b2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ use uucore::error::{FromIo, strip_errno}; | |
| use uucore::error::{UError, UResult, USimpleError, UUsageError}; | ||
| use uucore::extendedbigdecimal::ExtendedBigDecimal; | ||
| use uucore::format_usage; | ||
| use uucore::i18n::decimal::locale_decimal_separator; | ||
| use uucore::line_ending::LineEnding; | ||
| use uucore::parser::num_parser::{ExtendedParser, ExtendedParserError}; | ||
| use uucore::parser::parse_size::{ParseSizeError, Parser}; | ||
|
|
@@ -113,6 +114,24 @@ mod options { | |
|
|
||
| const DECIMAL_PT: u8 = b'.'; | ||
|
|
||
| fn locale_decimal_pt() -> u8 { | ||
| match locale_decimal_separator().as_bytes().first().copied() { | ||
| Some(b'.') => b'.', | ||
| Some(b',') => b',', | ||
| _ => DECIMAL_PT, | ||
| } | ||
| } | ||
|
|
||
| fn effective_decimal_pt(input: &[u8], locale_decimal: u8) -> u8 { | ||
| if locale_decimal == b',' { | ||
| let has_comma = input.contains(&b','); | ||
| if !has_comma && input.contains(&b'.') { | ||
| return b'.'; | ||
| } | ||
| } | ||
| locale_decimal | ||
| } | ||
|
|
||
| const NEGATIVE: &u8 = &b'-'; | ||
| const POSITIVE: &u8 = &b'+'; | ||
|
|
||
|
|
@@ -637,8 +656,9 @@ impl<'a> Line<'a> { | |
| } | ||
| SortMode::GeneralNumeric => { | ||
| let initial_selection = &self.line[selection.clone()]; | ||
|
|
||
| let leading = get_leading_gen(initial_selection); | ||
| let locale_decimal = locale_decimal_pt(); | ||
| let decimal_pt = effective_decimal_pt(initial_selection, locale_decimal); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can just be: An example of where this causes issues is:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be good to add these cases to the GNU test suite, will follow up with that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix it |
||
| let leading = get_leading_gen(initial_selection, decimal_pt); | ||
|
|
||
| // Shorten selection to leading. | ||
| selection.start += leading.start; | ||
|
|
@@ -965,7 +985,12 @@ impl FieldSelector { | |
| Selection::WithNumInfo(range_str, info) | ||
| } else if self.settings.mode == SortMode::GeneralNumeric { | ||
| // Parse this number as BigDecimal, as this is the requirement for general numeric sorting. | ||
| Selection::AsBigDecimal(general_bd_parse(&range_str[get_leading_gen(range_str)])) | ||
| let locale_decimal = locale_decimal_pt(); | ||
| let decimal_pt = effective_decimal_pt(range_str, locale_decimal); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here for the comment above, you can take that entire helper function out
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fix |
||
| Selection::AsBigDecimal(general_bd_parse( | ||
| &range_str[get_leading_gen(range_str, decimal_pt)], | ||
| decimal_pt, | ||
| )) | ||
| } else { | ||
| // This is not a numeric sort, so we don't need a NumCache. | ||
| Selection::Str(range_str) | ||
|
|
@@ -2020,7 +2045,7 @@ fn ascii_case_insensitive_cmp(a: &[u8], b: &[u8]) -> Ordering { | |
| // scientific notation, so we strip those lines only after the end of the following numeric string. | ||
| // For example, 5e10KFD would be 5e10 or 5x10^10 and +10000HFKJFK would become 10000. | ||
| #[allow(clippy::cognitive_complexity)] | ||
| fn get_leading_gen(inp: &[u8]) -> Range<usize> { | ||
| fn get_leading_gen(inp: &[u8], decimal_pt: u8) -> Range<usize> { | ||
| let trimmed = inp.trim_ascii_start(); | ||
| let leading_whitespace_len = inp.len() - trimmed.len(); | ||
|
|
||
|
|
@@ -2058,7 +2083,7 @@ fn get_leading_gen(inp: &[u8]) -> Range<usize> { | |
| continue; | ||
| } | ||
|
|
||
| if c == DECIMAL_PT && !had_decimal_pt && !had_e_notation { | ||
| if c == decimal_pt && !had_decimal_pt && !had_e_notation { | ||
| had_decimal_pt = true; | ||
| continue; | ||
| } | ||
|
|
@@ -2101,9 +2126,16 @@ pub enum GeneralBigDecimalParseResult { | |
| /// Parse the beginning string into a [`GeneralBigDecimalParseResult`]. | ||
| /// Using a [`GeneralBigDecimalParseResult`] instead of [`ExtendedBigDecimal`] is necessary to correctly order floats. | ||
| #[inline(always)] | ||
| fn general_bd_parse(a: &[u8]) -> GeneralBigDecimalParseResult { | ||
| fn general_bd_parse(a: &[u8], decimal_pt: u8) -> GeneralBigDecimalParseResult { | ||
| let parsed_bytes = (decimal_pt != DECIMAL_PT).then(|| { | ||
| a.iter() | ||
| .map(|&b| if b == decimal_pt { DECIMAL_PT } else { b }) | ||
| .collect::<Vec<_>>() | ||
| }); | ||
| let input = parsed_bytes.as_deref().unwrap_or(a); | ||
|
|
||
| // The string should be valid ASCII to be parsed. | ||
| let Ok(a) = std::str::from_utf8(a) else { | ||
| let Ok(a) = std::str::from_utf8(input) else { | ||
| return GeneralBigDecimalParseResult::Invalid; | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this function used? Shouldn't it always just default to the locale?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK fix