Skip to content

Commit

Permalink
refactor: Reduce code size of testing tokens if they're a number
Browse files Browse the repository at this point in the history
This commit is a tiny win in compiled code size of a final binary
including `clap` which shaves off 19k of compiled code locally.
Previously tokens were checked if they were a number by using
`.parse::<f64>().is_ok()`, but parsing floats is relatively heavyweight
in terms of code size. This replaces the check with a more naive "does
this string have lots of ascii digits" check where the compiled size of
this check should be much smaller.
  • Loading branch information
alexcrichton committed Oct 24, 2023
1 parent 1b84314 commit f7176d5
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 15 deletions.
6 changes: 3 additions & 3 deletions clap_builder/src/parser/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ impl<'cmd> Parser<'cmd> {

if self.cmd[current_positional.get_id()].is_allow_hyphen_values_set()
|| (self.cmd[current_positional.get_id()].is_allow_negative_numbers_set()
&& next.is_number())
&& next.looks_like_negative_number())
{
// If allow hyphen, this isn't a new arg.
debug!("Parser::is_new_arg: Allow hyphen");
Expand Down Expand Up @@ -841,7 +841,7 @@ impl<'cmd> Parser<'cmd> {

#[allow(clippy::blocks_in_if_conditions)]
if matches!(parse_state, ParseState::Opt(opt) | ParseState::Pos(opt)
if self.cmd[opt].is_allow_hyphen_values_set() || (self.cmd[opt].is_allow_negative_numbers_set() && short_arg.is_number()))
if self.cmd[opt].is_allow_hyphen_values_set() || (self.cmd[opt].is_allow_negative_numbers_set() && short_arg.looks_like_number()))
{
debug!("Parser::parse_short_args: prior arg accepts hyphenated values",);
return Ok(ParseResult::MaybeHyphenValue);
Expand All @@ -851,7 +851,7 @@ impl<'cmd> Parser<'cmd> {
.get(&pos_counter)
.map(|arg| arg.is_allow_negative_numbers_set())
.unwrap_or_default()
&& short_arg.is_number()
&& short_arg.looks_like_number()
{
debug!("Parser::parse_short_arg: negative number");
return Ok(ParseResult::MaybeHyphenValue);
Expand Down
28 changes: 23 additions & 5 deletions clap_lex/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,10 +300,14 @@ impl<'s> ParsedArg<'s> {
self.inner == "--"
}

/// Does the argument look like a number
pub fn is_number(&self) -> bool {
/// Does the argument look like a negative number?
///
/// This won't parse the number in full but attempts to see if this looks
/// like something along the lines of `-3`, `-0.3`, or `-33.03`
pub fn looks_like_negative_number(&self) -> bool {
self.to_value()
.map(|s| s.parse::<f64>().is_ok())
.ok()
.and_then(|s| Some(looks_like_number(s.strip_prefix('-')?)))
.unwrap_or_default()
}

Expand Down Expand Up @@ -408,8 +412,8 @@ impl<'s> ShortFlags<'s> {
/// Does the short flag look like a number
///
/// Ideally call this before doing any iterator
pub fn is_number(&self) -> bool {
self.invalid_suffix.is_none() && self.utf8_prefix.as_str().parse::<f64>().is_ok()
pub fn looks_like_number(&self) -> bool {
self.invalid_suffix.is_none() && looks_like_number(self.utf8_prefix.as_str())
}

/// Advance the iterator, returning the next short flag on success
Expand Down Expand Up @@ -466,3 +470,17 @@ fn split_nonutf8_once(b: &OsStr) -> (&str, Option<&OsStr>) {
}
}
}

fn looks_like_number(arg: &str) -> bool {
// Return true if this looks like an integer or a float where it's all
// digits plus an optional single dot after some digits.
let mut seen_dot = false;
for (i, c) in arg.as_bytes().iter().enumerate() {
match c {
b'0'..=b'9' => {}
b'.' if !seen_dot && i > 0 => seen_dot = true,
_ => return false,
}
}
true
}
6 changes: 3 additions & 3 deletions clap_lex/tests/parsed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ fn is_negative_number() {
assert_eq!(raw.next_os(&mut cursor), Some(std::ffi::OsStr::new("bin")));
let next = raw.next(&mut cursor).unwrap();

assert!(next.is_number());
assert!(next.looks_like_negative_number());
}

#[test]
Expand All @@ -135,7 +135,7 @@ fn is_positive_number() {
assert_eq!(raw.next_os(&mut cursor), Some(std::ffi::OsStr::new("bin")));
let next = raw.next(&mut cursor).unwrap();

assert!(next.is_number());
assert!(!next.looks_like_negative_number());
}

#[test]
Expand All @@ -145,7 +145,7 @@ fn is_not_number() {
assert_eq!(raw.next_os(&mut cursor), Some(std::ffi::OsStr::new("bin")));
let next = raw.next(&mut cursor).unwrap();

assert!(!next.is_number());
assert!(!next.looks_like_negative_number());
}

#[test]
Expand Down
8 changes: 4 additions & 4 deletions clap_lex/tests/shorts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,23 +151,23 @@ fn is_exhausted_empty() {
}

#[test]
fn is_number() {
fn looks_like_negative_number() {
let raw = clap_lex::RawArgs::new(["bin", "-1.0"]);
let mut cursor = raw.cursor();
assert_eq!(raw.next_os(&mut cursor), Some(std::ffi::OsStr::new("bin")));
let next = raw.next(&mut cursor).unwrap();
let shorts = next.to_short().unwrap();

assert!(shorts.is_number());
assert!(shorts.looks_like_number());
}

#[test]
fn is_not_number() {
fn is_not_negaitve_number() {
let raw = clap_lex::RawArgs::new(["bin", "-hello"]);
let mut cursor = raw.cursor();
assert_eq!(raw.next_os(&mut cursor), Some(std::ffi::OsStr::new("bin")));
let next = raw.next(&mut cursor).unwrap();
let shorts = next.to_short().unwrap();

assert!(!shorts.is_number());
assert!(!shorts.looks_like_number());
}

0 comments on commit f7176d5

Please sign in to comment.