From a77848fb830e4e79a9aa1e9aa62cf8d7eb26242c Mon Sep 17 00:00:00 2001 From: kf zheng <100595273+Kev1n8@users.noreply.github.com> Date: Sat, 13 Jul 2024 16:19:01 +0800 Subject: [PATCH] printf: Fix extra padding (#6548) --- .../src/lib/features/format/num_format.rs | 100 ++++++++++---- src/uucore/src/lib/features/format/spec.rs | 6 +- tests/by-util/test_printf.rs | 124 ++++++++++++++++++ 3 files changed, 200 insertions(+), 30 deletions(-) diff --git a/src/uucore/src/lib/features/format/num_format.rs b/src/uucore/src/lib/features/format/num_format.rs index 52551f10b..676a1065d 100644 --- a/src/uucore/src/lib/features/format/num_format.rs +++ b/src/uucore/src/lib/features/format/num_format.rs @@ -5,6 +5,7 @@ //! Utilities for formatting numbers in various formats +use std::cmp::min; use std::io::Write; use super::{ @@ -77,22 +78,16 @@ pub struct SignedInt { impl Formatter for SignedInt { type Input = i64; - fn fmt(&self, mut writer: impl Write, x: Self::Input) -> std::io::Result<()> { - if x >= 0 { - match self.positive_sign { - PositiveSign::None => Ok(()), - PositiveSign::Plus => write!(writer, "+"), - PositiveSign::Space => write!(writer, " "), - }?; - } + fn fmt(&self, writer: impl Write, x: Self::Input) -> std::io::Result<()> { + let s = if self.precision > 0 { + format!("{:0>width$}", x.abs(), width = self.precision) + } else { + x.abs().to_string() + }; - let s = format!("{:0width$}", x, width = self.precision); + let sign_indicator = get_sign_indicator(self.positive_sign, &x); - match self.alignment { - NumberAlignment::Left => write!(writer, "{s: write!(writer, "{s:>width$}", width = self.width), - NumberAlignment::RightZero => write!(writer, "{s:0>width$}", width = self.width), - } + write_output(writer, sign_indicator, s, self.width, self.alignment) } fn try_from_spec(s: Spec) -> Result { @@ -244,16 +239,8 @@ impl Default for Float { impl Formatter for Float { type Input = f64; - fn fmt(&self, mut writer: impl Write, x: Self::Input) -> std::io::Result<()> { - if x.is_sign_positive() { - match self.positive_sign { - PositiveSign::None => Ok(()), - PositiveSign::Plus => write!(writer, "+"), - PositiveSign::Space => write!(writer, " "), - }?; - } - - let s = if x.is_finite() { + fn fmt(&self, writer: impl Write, x: Self::Input) -> std::io::Result<()> { + let mut s = if x.is_finite() { match self.variant { FloatVariant::Decimal => { format_float_decimal(x, self.precision, self.force_decimal) @@ -272,11 +259,13 @@ impl Formatter for Float { format_float_non_finite(x, self.case) }; - match self.alignment { - NumberAlignment::Left => write!(writer, "{s: write!(writer, "{s:>width$}", width = self.width), - NumberAlignment::RightZero => write!(writer, "{s:0>width$}", width = self.width), - } + // The format function will parse `x` together with its sign char, + // which should be placed in `sign_indicator`. So drop it here + s = if x < 0. { s[1..].to_string() } else { s }; + + let sign_indicator = get_sign_indicator(self.positive_sign, &x); + + write_output(writer, sign_indicator, s, self.width, self.alignment) } fn try_from_spec(s: Spec) -> Result @@ -326,6 +315,18 @@ impl Formatter for Float { } } +fn get_sign_indicator(sign: PositiveSign, x: &T) -> String { + if *x >= T::default() { + match sign { + PositiveSign::None => String::new(), + PositiveSign::Plus => String::from("+"), + PositiveSign::Space => String::from(" "), + } + } else { + String::from("-") + } +} + fn format_float_non_finite(f: f64, case: Case) -> String { debug_assert!(!f.is_finite()); let mut s = format!("{f}"); @@ -501,6 +502,47 @@ fn strip_fractional_zeroes_and_dot(s: &mut String) { } } +fn write_output( + mut writer: impl Write, + sign_indicator: String, + mut s: String, + width: usize, + alignment: NumberAlignment, +) -> std::io::Result<()> { + // Take length of `sign_indicator`, which could be 0 or 1, into consideration when padding + // by storing remaining_width indicating the actual width needed. + // Using min() because self.width could be 0, 0usize - 1usize should be avoided + let remaining_width = width - min(width, sign_indicator.len()); + match alignment { + NumberAlignment::Left => write!( + writer, + "{sign_indicator}{s: { + let is_sign = sign_indicator.starts_with('-') || sign_indicator.starts_with('+'); // When sign_indicator is in ['-', '+'] + if is_sign && remaining_width > 0 { + // Make sure sign_indicator is just next to number, e.g. "% +5.1f" 1 ==> $ +1.0 + s = sign_indicator + s.as_str(); + write!(writer, "{s:>width$}", width = remaining_width + 1) // Since we now add sign_indicator and s together, plus 1 + } else { + write!( + writer, + "{sign_indicator}{s:>width$}", + width = remaining_width + ) + } + } + NumberAlignment::RightZero => { + write!( + writer, + "{sign_indicator}{s:0>width$}", + width = remaining_width + ) + } + } +} + #[cfg(test)] mod test { use crate::format::num_format::{Case, ForceDecimal}; diff --git a/src/uucore/src/lib/features/format/spec.rs b/src/uucore/src/lib/features/format/spec.rs index 29e696d32..581e1fa06 100644 --- a/src/uucore/src/lib/features/format/spec.rs +++ b/src/uucore/src/lib/features/format/spec.rs @@ -252,7 +252,11 @@ impl Spec { } else { Case::Lowercase }, - alignment, + alignment: if flags.zero && !flags.minus { + NumberAlignment::RightZero // float should always try to zero pad despite the precision + } else { + alignment + }, positive_sign, }, _ => return Err(&start[..index]), diff --git a/tests/by-util/test_printf.rs b/tests/by-util/test_printf.rs index 26f556e8e..29a8cc914 100644 --- a/tests/by-util/test_printf.rs +++ b/tests/by-util/test_printf.rs @@ -792,3 +792,127 @@ fn float_invalid_precision_fails() { .fails() .stderr_is("printf: invalid precision: '2147483648'\n"); } + +// The following padding-tests test for the cases in which flags in ['0', ' '] are given. +// For integer, only try to pad when no precision is given, while +// for float, always try to pad +#[test] +fn space_padding_with_space_test() { + // Check if printf gives an extra space in the beginning + new_ucmd!() + .args(&["% 3d", "1"]) + .succeeds() + .stdout_only(" 1"); +} + +#[test] +fn zero_padding_with_space_test() { + new_ucmd!() + .args(&["% 03d", "1"]) + .succeeds() + .stdout_only(" 01"); +} + +#[test] +fn zero_padding_with_plus_test() { + new_ucmd!() + .args(&["%+04d", "1"]) + .succeeds() + .stdout_only("+001"); +} + +#[test] +fn negative_zero_padding_test() { + new_ucmd!() + .args(&["%03d", "-1"]) + .succeeds() + .stdout_only("-01"); +} + +#[test] +fn negative_zero_padding_with_space_test() { + new_ucmd!() + .args(&["% 03d", "-1"]) + .succeeds() + .stdout_only("-01"); +} + +#[test] +fn float_with_zero_precision_should_pad() { + new_ucmd!() + .args(&["%03.0f", "-1"]) + .succeeds() + .stdout_only("-01"); +} + +#[test] +fn precision_check() { + new_ucmd!() + .args(&["%.3d", "1"]) + .succeeds() + .stdout_only("001"); +} + +#[test] +fn space_padding_with_precision() { + new_ucmd!() + .args(&["%4.3d", "1"]) + .succeeds() + .stdout_only(" 001"); +} + +#[test] +fn float_zero_padding_with_precision() { + new_ucmd!() + .args(&["%04.1f", "1"]) + .succeeds() + .stdout_only("01.0"); +} + +#[test] +fn float_space_padding_with_precision() { + new_ucmd!() + .args(&["%4.1f", "1"]) + .succeeds() + .stdout_only(" 1.0"); +} + +#[test] +fn negative_float_zero_padding_with_precision() { + new_ucmd!() + .args(&["%05.1f", "-1"]) + .succeeds() + .stdout_only("-01.0"); +} + +#[test] +fn float_default_precision_space_padding() { + new_ucmd!() + .args(&["%10f", "1"]) + .succeeds() + .stdout_only(" 1.000000"); +} + +#[test] +fn float_default_precision_zero_padding() { + new_ucmd!() + .args(&["%010f", "1"]) + .succeeds() + .stdout_only("001.000000"); +} + +#[test] +fn flag_position_space_padding() { + new_ucmd!() + .args(&["% +3.1d", "1"]) + .succeeds() + .stdout_only(" +1"); +} + +#[test] +fn float_flag_position_space_padding() { + new_ucmd!() + .args(&["% +5.1f", "1"]) + .succeeds() + .stdout_only(" +1.0"); +}