Skip to content

Commit

Permalink
perf(parser): Take up less memory with ArgAction::Count
Browse files Browse the repository at this point in the history
Someone should not reasonably expect a coun flag to go up to billions,
millions, or even thousands.  255 should be sufficient for anyone,
right?

The original type was selected to be consistent with
`ArgMatches::occurrences_of` but that is also used for tracking how
many values appear which can be large with `xargs`.

I'm still conflicted on what the "right type" is an wish we could
support any numeric type.  When I did a search on github though, every
case was for debug/quiet flags and only supported 2-3 occurrences,
making a `u8` overkill.

This came out of a discussion on clap-rs#3792
  • Loading branch information
epage committed Jun 9, 2022
1 parent acfb130 commit 31b22d1
Show file tree
Hide file tree
Showing 16 changed files with 88 additions and 80 deletions.
4 changes: 2 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ Replaced
- `Arg::validator_regex` with users providing their own `builder::TypedValueParser` (#3756)
- `Arg::forbid_empty_values` with `builder::NonEmptyStringValueParser` / `builder::PathBufValueParser` (#3753)
- `Arg::possible_values` with `Arg::value_parser([...])`, `builder::PossibleValuesParser`, or `builder::EnumValueParser` (#3753)
- `Arg::max_occurrences` with `arg.action(ArgAction::Count).value_parser(value_parser!(u64).range(..N))` for flags (#3797)
- `Arg::max_occurrences` with `arg.action(ArgAction::Count).value_parser(value_parser!(u8).range(..N))` for flags (#3797)
- `Arg::multiple_occurrences` with `ArgAction::Append` or `ArgAction::Count` though positionals will need `Arg::multiple_values` (#3772, #3797)
- `Command::args_override_self` with `ArgAction::Set` (#2627, #3797)
- `AppSettings::NoAutoVersion` with `ArgAction` or `Command::disable_version_flag` (#3800)
Expand All @@ -99,7 +99,7 @@ Replaced
- `ArgAction::StoreValue` with `ArgAction::Set` or `ArgAction::Append` (#3797)
- `ArgAction::IncOccurrences` with `ArgAction::SetTrue` or `ArgAction::Count` (#3797)
- *(derive)* `#[clap(parse(from_flag))]` replaced with `#[clap(action = ArgAction::SetTrue)]` (#3794)
- *(derive)* `#[clap(parse(from_occurrences))]` replaced with `#[clap(action = ArgAction::Count)]` though the field's type must be `u64` (#3794)
- *(derive)* `#[clap(parse(from_occurrences))]` replaced with `#[clap(action = ArgAction::Count)]` though the field's type must be `u8` (#3794)
- *(derive)* `#[clap(parse(...))]` replaced with `#[clap(value_parser)]` (#3589, #3794)

## [3.1.18] - 2022-05-10
Expand Down
2 changes: 1 addition & 1 deletion examples/tutorial_builder/01_quick.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ fn main() {
// You can see how many times a particular flag or argument occurred
// Note, only flags can have multiple occurrences
match matches
.get_one::<u64>("debug")
.get_one::<u8>("debug")
.expect("Count's are defaulted")
{
0 => println!("Debug mode is off"),
Expand Down
2 changes: 1 addition & 1 deletion examples/tutorial_builder/03_01_flag_count.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ fn main() {
println!(
"verbose: {:?}",
matches
.get_one::<u64>("verbose")
.get_one::<u8>("verbose")
.expect("Count always defaulted")
);
}
2 changes: 1 addition & 1 deletion examples/tutorial_derive/01_quick.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ struct Cli {

/// Turn debugging information on
#[clap(short, long, action = clap::ArgAction::Count)]
debug: u64,
debug: u8,

#[clap(subcommand)]
command: Option<Commands>,
Expand Down
2 changes: 1 addition & 1 deletion examples/tutorial_derive/03_01_flag_count.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use clap::Parser;
#[clap(author, version, about, long_about = None)]
struct Cli {
#[clap(short, long, action = clap::ArgAction::Count)]
verbose: u64,
verbose: u8,
}

fn main() {
Expand Down
10 changes: 5 additions & 5 deletions src/builder/action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ pub enum ArgAction {
/// );
/// ```
SetFalse,
/// When encountered, increment a `u64` counter
/// When encountered, increment a `u8` counter
///
/// If no [`default_value`][super::Arg::default_value] is set, it will be `0`.
///
Expand All @@ -174,15 +174,15 @@ pub enum ArgAction {
/// assert!(matches.is_present("flag"));
/// assert_eq!(matches.occurrences_of("flag"), 0);
/// assert_eq!(
/// matches.get_one::<u64>("flag").copied(),
/// matches.get_one::<u8>("flag").copied(),
/// Some(2)
/// );
///
/// let matches = cmd.try_get_matches_from(["mycmd"]).unwrap();
/// assert!(matches.is_present("flag"));
/// assert_eq!(matches.occurrences_of("flag"), 0);
/// assert_eq!(
/// matches.get_one::<u64>("flag").copied(),
/// matches.get_one::<u8>("flag").copied(),
/// Some(0)
/// );
/// ```
Expand Down Expand Up @@ -287,7 +287,7 @@ impl ArgAction {
Self::IncOccurrence => None,
Self::SetTrue => Some(super::ValueParser::bool()),
Self::SetFalse => Some(super::ValueParser::bool()),
Self::Count => Some(crate::value_parser!(u64).into()),
Self::Count => Some(crate::value_parser!(u8).into()),
Self::Help => None,
Self::Version => None,
}
Expand All @@ -313,4 +313,4 @@ impl ArgAction {
}
}

pub(crate) type CountType = u64;
pub(crate) type CountType = u8;
4 changes: 2 additions & 2 deletions src/builder/arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -788,12 +788,12 @@ impl<'help> Arg<'help> {
}
}

/// Deprecated, for flags this is replaced with `action(ArgAction::Count).value_parser(value_parser!(u64).range(..max))`
/// Deprecated, for flags this is replaced with `action(ArgAction::Count).value_parser(value_parser!(u8).range(..max))`
#[inline]
#[must_use]
#[deprecated(
since = "3.2.0",
note = "For flags, replaced with `action(ArgAction::Count).value_parser(value_parser!(u64).range(..max))`"
note = "For flags, replaced with `action(ArgAction::Count).value_parser(value_parser!(u8).range(..max))`"
)]
pub fn max_occurrences(mut self, qty: usize) -> Self {
self.max_occurs = Some(qty);
Expand Down
38 changes: 19 additions & 19 deletions tests/builder/action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ fn count() {
let cmd = Command::new("test").arg(Arg::new("mammal").long("mammal").action(ArgAction::Count));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -391,7 +391,7 @@ fn count() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -403,7 +403,7 @@ fn count() {
.clone()
.try_get_matches_from(["test", "--mammal", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 2);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 2);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -425,7 +425,7 @@ fn count_with_explicit_default_value() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -434,7 +434,7 @@ fn count_with_explicit_default_value() {
assert_eq!(matches.index_of("mammal"), Some(1));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 10);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -455,19 +455,19 @@ fn count_with_default_value_if_present() {
.arg(Arg::new("dog").long("dog").action(ArgAction::Count));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 10);

let matches = cmd
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
}

#[test]
Expand All @@ -482,24 +482,24 @@ fn count_with_default_value_if_value() {
.arg(Arg::new("dog").long("dog").action(ArgAction::Count));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);

let matches = cmd
.clone()
.try_get_matches_from(["test", "--dog", "--dog"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 2);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 2);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 10);

let matches = cmd
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
}
38 changes: 19 additions & 19 deletions tests/builder/legacy/action.rs
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ fn count() {
let cmd = Command::new("test").arg(Arg::new("mammal").long("mammal").action(ArgAction::Count));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -391,7 +391,7 @@ fn count() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -403,7 +403,7 @@ fn count() {
.clone()
.try_get_matches_from(["test", "--mammal", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 2);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 2);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -425,7 +425,7 @@ fn count_with_explicit_default_value() {
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -434,7 +434,7 @@ fn count_with_explicit_default_value() {
assert_eq!(matches.index_of("mammal"), Some(1));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 10);
assert_eq!(matches.is_present("mammal"), true);
#[allow(deprecated)]
{
Expand All @@ -455,19 +455,19 @@ fn count_with_default_value_if_present() {
.arg(Arg::new("dog").long("dog").action(ArgAction::Count));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 10);

let matches = cmd
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
}

#[test]
Expand All @@ -482,24 +482,24 @@ fn count_with_default_value_if_value() {
.arg(Arg::new("dog").long("dog").action(ArgAction::Count));

let matches = cmd.clone().try_get_matches_from(["test"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);

let matches = cmd.clone().try_get_matches_from(["test", "--dog"]).unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 0);

let matches = cmd
.clone()
.try_get_matches_from(["test", "--dog", "--dog"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 2);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 10);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 2);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 10);

let matches = cmd
.clone()
.try_get_matches_from(["test", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<u64>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u64>("mammal").unwrap(), 1);
assert_eq!(*matches.get_one::<u8>("dog").unwrap(), 0);
assert_eq!(*matches.get_one::<u8>("mammal").unwrap(), 1);
}
Loading

0 comments on commit 31b22d1

Please sign in to comment.