Skip to content

printf: use non-zero indexes #7855

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.

Sign up for GitHub

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

Merged
RenjiSann merged 1 commit into from
Apr 30, 2025
Merged

printf: use non-zero indexes #7855

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 28 additions & 23 deletions src/uucore/src/lib/features/format/argument.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use crate::{
show_error, show_warning,
};
use os_display::Quotable;
use std::ffi::OsStr;
use std::{ffi::OsStr, num::NonZero};

/// An argument for formatting
///
Expand Down Expand Up @@ -129,8 +129,9 @@ impl<'a> FormatArguments<'a> {
}
}

fn get_at_relative_position(&mut self, pos: usize) -> Option<&'a FormatArgument> {
let pos = pos.saturating_sub(1).saturating_add(self.current_offset);
fn get_at_relative_position(&mut self, pos: NonZero<usize>) -> Option<&'a FormatArgument> {
let pos: usize = pos.into();
let pos = (pos - 1).saturating_add(self.current_offset);
self.highest_arg_position = Some(self.highest_arg_position.map_or(pos, |x| x.max(pos)));
self.args.get(pos)
}
Expand Down Expand Up @@ -281,6 +282,10 @@ mod tests {
assert!(args.is_exhausted());
}

fn non_zero_pos(n: usize) -> ArgumentLocation {
ArgumentLocation::Position(NonZero::new(n).unwrap())
}

#[test]
fn test_position_access_pattern() {
// Test with consistent positional access patterns
Expand All @@ -297,23 +302,23 @@ mod tests {
]);

// First batch - positional access
assert_eq!(b'b', args.next_char(&ArgumentLocation::Position(2))); // Position 2
assert_eq!(b'a', args.next_char(&ArgumentLocation::Position(1))); // Position 1
assert_eq!(b'c', args.next_char(&ArgumentLocation::Position(3))); // Position 3
assert_eq!(b'b', args.next_char(&non_zero_pos(2))); // Position 2
assert_eq!(b'a', args.next_char(&non_zero_pos(1))); // Position 1
assert_eq!(b'c', args.next_char(&non_zero_pos(3))); // Position 3
args.start_next_batch();
assert!(!args.is_exhausted());

// Second batch - same positional pattern
assert_eq!(b'e', args.next_char(&ArgumentLocation::Position(2))); // Position 2
assert_eq!(b'd', args.next_char(&ArgumentLocation::Position(1))); // Position 1
assert_eq!(b'f', args.next_char(&ArgumentLocation::Position(3))); // Position 3
assert_eq!(b'e', args.next_char(&non_zero_pos(2))); // Position 2
assert_eq!(b'd', args.next_char(&non_zero_pos(1))); // Position 1
assert_eq!(b'f', args.next_char(&non_zero_pos(3))); // Position 3
args.start_next_batch();
assert!(!args.is_exhausted());

// Third batch - same positional pattern (last batch)
assert_eq!(b'h', args.next_char(&ArgumentLocation::Position(2))); // Position 2
assert_eq!(b'g', args.next_char(&ArgumentLocation::Position(1))); // Position 1
assert_eq!(b'i', args.next_char(&ArgumentLocation::Position(3))); // Position 3
assert_eq!(b'h', args.next_char(&non_zero_pos(2))); // Position 2
assert_eq!(b'g', args.next_char(&non_zero_pos(1))); // Position 1
assert_eq!(b'i', args.next_char(&non_zero_pos(3))); // Position 3
args.start_next_batch();
assert!(args.is_exhausted());
}
Expand All @@ -334,19 +339,19 @@ mod tests {

// First batch - mix of sequential and positional
assert_eq!(b'a', args.next_char(&ArgumentLocation::NextArgument)); // Sequential
assert_eq!(b'c', args.next_char(&ArgumentLocation::Position(3))); // Positional
assert_eq!(b'c', args.next_char(&non_zero_pos(3))); // Positional
args.start_next_batch();
assert!(!args.is_exhausted());

// Second batch - same mixed pattern
assert_eq!(b'd', args.next_char(&ArgumentLocation::NextArgument)); // Sequential
assert_eq!(b'f', args.next_char(&ArgumentLocation::Position(3))); // Positional
assert_eq!(b'f', args.next_char(&non_zero_pos(3))); // Positional
args.start_next_batch();
assert!(!args.is_exhausted());

// Last batch - same mixed pattern
assert_eq!(b'g', args.next_char(&ArgumentLocation::NextArgument)); // Sequential
assert_eq!(b'\0', args.next_char(&ArgumentLocation::Position(3))); // Out of bounds
assert_eq!(b'\0', args.next_char(&non_zero_pos(3))); // Out of bounds
args.start_next_batch();
assert!(args.is_exhausted());
}
Expand Down Expand Up @@ -419,16 +424,16 @@ mod tests {
let mut args = FormatArguments::new(&args);

// First batch - positional access of different types
assert_eq!(b'a', args.next_char(&ArgumentLocation::Position(1)));
assert_eq!("test", args.next_string(&ArgumentLocation::Position(2)));
assert_eq!(42, args.next_u64(&ArgumentLocation::Position(3)));
assert_eq!(b'a', args.next_char(&non_zero_pos(1)));
assert_eq!("test", args.next_string(&non_zero_pos(2)));
assert_eq!(42, args.next_u64(&non_zero_pos(3)));
args.start_next_batch();
assert!(!args.is_exhausted());

// Second batch - same pattern
assert_eq!(b'b', args.next_char(&ArgumentLocation::Position(1)));
assert_eq!("more", args.next_string(&ArgumentLocation::Position(2)));
assert_eq!(99, args.next_u64(&ArgumentLocation::Position(3)));
assert_eq!(b'b', args.next_char(&non_zero_pos(1)));
assert_eq!("more", args.next_string(&non_zero_pos(2)));
assert_eq!(99, args.next_u64(&non_zero_pos(3)));
args.start_next_batch();
assert!(args.is_exhausted());
}
Expand All @@ -446,13 +451,13 @@ mod tests {

// First batch
assert_eq!(b'a', args.next_char(&ArgumentLocation::NextArgument));
assert_eq!(b'c', args.next_char(&ArgumentLocation::Position(3)));
assert_eq!(b'c', args.next_char(&non_zero_pos(3)));
args.start_next_batch();
assert!(!args.is_exhausted());

// Second batch (partial)
assert_eq!(b'd', args.next_char(&ArgumentLocation::NextArgument));
assert_eq!(b'\0', args.next_char(&ArgumentLocation::Position(3))); // Out of bounds
assert_eq!(b'\0', args.next_char(&non_zero_pos(3))); // Out of bounds
args.start_next_batch();
assert!(args.is_exhausted());
}
Expand Down
26 changes: 16 additions & 10 deletions src/uucore/src/lib/features/format/spec.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use super::{
parse_escape_only,
};
use crate::format::FormatArguments;
use std::{io::Write, ops::ControlFlow};
use std::{io::Write, num::NonZero, ops::ControlFlow};

/// A parsed specification for formatting a value
///
Expand Down Expand Up @@ -70,7 +70,7 @@ pub enum Spec {
#[derive(Clone, Copy, Debug)]
pub enum ArgumentLocation {
NextArgument,
Position(usize),
Position(NonZero<usize>),
}

/// Precision and width specified might use an asterisk to indicate that they are
Expand Down Expand Up @@ -156,7 +156,9 @@ impl Spec {
let start = *rest;

// Check for a positional specifier (%m$)
let position = eat_argument_position(rest, &mut index);
let Some(position) = eat_argument_position(rest, &mut index) else {
return Err(&start[..index]);
};

let flags = Flags::parse(rest, &mut index);

Expand Down Expand Up @@ -566,27 +568,27 @@ fn write_padded(
}

// Check for a number ending with a '$'
fn eat_argument_position(rest: &mut &[u8], index: &mut usize) -> ArgumentLocation {
fn eat_argument_position(rest: &mut &[u8], index: &mut usize) -> Option<ArgumentLocation> {
let original_index = *index;
if let Some(pos) = eat_number(rest, index) {
if let Some(&b'$') = rest.get(*index) {
*index += 1;
ArgumentLocation::Position(pos)
Some(ArgumentLocation::Position(NonZero::new(pos)?))
} else {
*index = original_index;
ArgumentLocation::NextArgument
Some(ArgumentLocation::NextArgument)
}
} else {
*index = original_index;
ArgumentLocation::NextArgument
Some(ArgumentLocation::NextArgument)
}
}

fn eat_asterisk_or_number(rest: &mut &[u8], index: &mut usize) -> Option<CanAsterisk<usize>> {
if let Some(b'*') = rest.get(*index) {
*index += 1;
// Check for a positional specifier (*m$)
Some(CanAsterisk::Asterisk(eat_argument_position(rest, index)))
Some(CanAsterisk::Asterisk(eat_argument_position(rest, index)?))
} else {
eat_number(rest, index).map(CanAsterisk::Fixed)
}
Expand Down Expand Up @@ -670,7 +672,9 @@ mod tests {
assert_eq!(
Some((2, false)),
resolve_asterisk_width(
Some(CanAsterisk::Asterisk(ArgumentLocation::Position(2))),
Some(CanAsterisk::Asterisk(ArgumentLocation::Position(
NonZero::new(2).unwrap()
))),
&mut FormatArguments::new(&[
FormatArgument::Unparsed("1".to_string()),
FormatArgument::Unparsed("2".to_string()),
Expand Down Expand Up @@ -738,7 +742,9 @@ mod tests {
assert_eq!(
Some(2),
resolve_asterisk_precision(
Some(CanAsterisk::Asterisk(ArgumentLocation::Position(2))),
Some(CanAsterisk::Asterisk(ArgumentLocation::Position(
NonZero::new(2).unwrap()
))),
&mut FormatArguments::new(&[
FormatArgument::Unparsed("1".to_string()),
FormatArgument::Unparsed("2".to_string()),
Expand Down
5 changes: 5 additions & 0 deletions tests/by-util/test_printf.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,11 @@ fn positional_format_specifiers() {
.succeeds()
.stdout_only("05-");

new_ucmd!()
.args(&["%0$d%d-", "5", "10", "6", "20"])
.fails_with_code(1)
.stderr_only("printf: %0$: invalid conversion specification\n");

new_ucmd!()
.args(&[
"Octal: %6$o, Int: %1$d, Float: %4$f, String: %2$s, Hex: %7$x, Scientific: %5$e, Char: %9$c, Unsigned: %3$u, Integer: %8$i",
Expand Down
Loading