diff --git a/src/build/app/mod.rs b/src/build/app/mod.rs index 096b4032..0968b5a5 100644 --- a/src/build/app/mod.rs +++ b/src/build/app/mod.rs @@ -639,7 +639,7 @@ impl<'a, 'b> App<'a, 'b> { None }; let arg = a.into().help_heading(help_heading); - self.args.make_entries(arg); + self.args.push(arg); self } @@ -679,7 +679,7 @@ impl<'a, 'b> App<'a, 'b> { // @TODO @perf @p4 @v3-beta: maybe extend_from_slice would be possible and perform better? // But that may also not let us do `&["-a 'some'", "-b 'other']` because of not Into for arg in args.into_iter() { - self.args.make_entries(arg.into()); + self.args.push(arg.into()); } self } @@ -988,7 +988,11 @@ impl<'a, 'b> App<'a, 'b> { where F: FnOnce(Arg<'a, 'b>) -> Arg<'a, 'b>, { - self.args.mut_arg(arg, f); + let a = self + .args + .remove_by_name(arg) + .expect(&*format!("Arg '{}' not found.", arg)); + self.args.push(f(a)); self } @@ -1461,6 +1465,7 @@ impl<'a, 'b> App<'a, 'b> { } debug_assert!(self._app_debug_asserts()); + self.args._build(); self.settings.set(AppSettings::Propagated); } @@ -1508,7 +1513,7 @@ impl<'a, 'b> App<'a, 'b> { } { for a in self.args.values().filter(|a| a.is_set(ArgSettings::Global)) { - sc.args.make_entries(a.clone()); + sc.args.push(a.clone()); } } // @TODO @deadcode @perf @v3-alpha: Currently we're not propagating @@ -1521,7 +1526,12 @@ impl<'a, 'b> App<'a, 'b> { pub(crate) fn _create_help_and_version(&mut self) { debugln!("App::_create_help_and_version;"); // name is "hclap_help" because flags are sorted by name - if !self.contains_long("help") { + if !self + .args + .values() + .filter_map(|x| x.long) + .any(|x| x == "help") + { debugln!("App::_create_help_and_version: Building --help"); if self.help_short.is_none() && !self.contains_short('h') { self.help_short = Some('h'); @@ -1532,11 +1542,16 @@ impl<'a, 'b> App<'a, 'b> { // we have to set short manually because we're dealing with char's arg.short = self.help_short; - self.args.make_entries(arg); + self.args.push(arg); } else { self.settings.unset(AppSettings::NeedsLongHelp); } - if !self.is_set(AppSettings::DisableVersion) && !self.contains_long("version") { + if !self.is_set(AppSettings::DisableVersion) && !self + .args + .values() + .filter_map(|x| x.long) + .any(|x| x == "version") + { debugln!("App::_create_help_and_version: Building --version"); if self.version_short.is_none() && !self.contains_short('V') { self.version_short = Some('V'); @@ -1547,7 +1562,7 @@ impl<'a, 'b> App<'a, 'b> { .help(self.version_message.unwrap_or("Prints version information")); // we have to set short manually because we're dealing with char's arg.short = self.version_short; - self.args.make_entries(arg); + self.args.push(arg); } else { self.settings.unset(AppSettings::NeedsLongVersion); } @@ -1594,10 +1609,11 @@ impl<'a, 'b> App<'a, 'b> { // Long conflicts if let Some(l) = a.long { assert!( - args!(self).fold( - 0, - |acc, arg| if arg.long == Some(l) { acc + 1 } else { acc }, - ) < 2, + args!(self).fold(0, |acc, arg| if arg.long == Some(l) { + acc + 1 + } else { + acc + },) < 2, "Argument long must be unique\n\n\t--{} is already in use", l ); @@ -1606,10 +1622,11 @@ impl<'a, 'b> App<'a, 'b> { // Short conflicts if let Some(s) = a.short { assert!( - args!(self).fold( - 0, - |acc, arg| if arg.short == Some(s) { acc + 1 } else { acc }, - ) < 2, + args!(self).fold(0, |acc, arg| if arg.short == Some(s) { + acc + 1 + } else { + acc + },) < 2, "Argument short must be unique\n\n\t-{} is already in use", s ); @@ -1618,12 +1635,10 @@ impl<'a, 'b> App<'a, 'b> { if let Some(idx) = a.index { // No index conflicts assert!( - positionals!(self).fold(0, |acc, p| { - if p.index == Some(idx as u64) { - acc + 1 - } else { - acc - } + positionals!(self).fold(0, |acc, p| if p.index == Some(idx as u64) { + acc + 1 + } else { + acc }) < 2, "Argument '{}' has the same index as another positional \ argument\n\n\tUse Arg::setting(ArgSettings::MultipleValues) to allow one \ @@ -1718,8 +1733,7 @@ impl<'a, 'b> App<'a, 'b> { } else { x.to_string() } - }) - .collect::>() + }).collect::>() .join("|"); format!("<{}>", &*g_string) } @@ -1748,7 +1762,11 @@ impl<'a, 'b> App<'a, 'b> { ColorWhen::Auto } } + pub(crate) fn contains_long(&self, l: &str) -> bool { + if !self.is_set(AppSettings::Propagated) { + panic!("If App::_build hasn't been called, manually search through Arg longs"); + } longs!(self).any(|al| al == &OsString::from(l)) } @@ -1926,7 +1944,7 @@ impl<'a, 'b> App<'a, 'b> { /// **Deprecated:** Use #[deprecated( since = "2.30.0", - note = "Use `App::mut_arg(\"help\", |a| a.short(\"H\"))` instead. Will be removed in v3.0-beta" + note = "Build and Arg with a long of '--help' to override the default help arg instead. Will be removed in v3.0-beta" )] pub fn help_short + 'b>(mut self, s: S) -> Self { let c = s @@ -1942,7 +1960,7 @@ impl<'a, 'b> App<'a, 'b> { /// **Deprecated:** Use #[deprecated( since = "2.30.0", - note = "Use `App::mut_arg(\"version\", |a| a.short(\"v\"))` instead. Will be removed in v3.0-beta" + note = "Build and Arg with a long of '--version' to override the default version arg instead. Will be removed in v3.0-beta" )] pub fn version_short>(mut self, s: S) -> Self { let c = s @@ -2011,7 +2029,7 @@ impl<'a, 'b> App<'a, 'b> { note = "Use `App::arg(Arg::from(&str)` instead. Will be removed in v3.0-beta" )] pub fn arg_from_usage(mut self, usage: &'a str) -> Self { - self.args.make_entries(Arg::from(usage)); + self.args.push(Arg::from(usage)); self } @@ -2026,7 +2044,7 @@ impl<'a, 'b> App<'a, 'b> { if l.is_empty() { continue; } - self.args.make_entries(Arg::from(l)); + self.args.push(Arg::from(l)); } self } diff --git a/src/mkeymap.rs b/src/mkeymap.rs index 62f73a66..f2b4bfc8 100644 --- a/src/mkeymap.rs +++ b/src/mkeymap.rs @@ -1,17 +1,15 @@ use build::Arg; use std::collections::hash_map; -use std::collections::hash_map::DefaultHasher; use std::collections::{HashMap, HashSet}; use std::ffi::OsString; -use std::hash::{Hash, Hasher}; +use std::hash::Hash; use std::slice; -// ! rustdoc #[derive(Default, PartialEq, Debug, Clone)] pub struct MKeyMap { keys: HashMap, value_index: Vec, - values: HashMap>, + built: bool, // mutation isn't possible after being built } #[derive(Debug, PartialEq, Eq, Hash, Clone)] @@ -57,40 +55,19 @@ where } pub fn push(&mut self, value: T) -> usize { - let index; - let mut hasher = DefaultHasher::new(); - - value.hash(&mut hasher); - - let hash = hasher.finish(); - - if let Some((idx, _)) = self.values.get(&hash).and_then(|ids| { - ids.iter() - .map(|&x| (x, &self.value_index[x])) - .find(|(_i, x)| x == &&value) - }) { - debug_assert!(false, "Non-unique value found"); - index = idx; - } else { - self.value_index.push(value); - index = self.value_index.len() - 1; - self.values - .entry(hash) - .and_modify(|x| { - x.insert(index); - }).or_insert({ - let mut set = HashSet::new(); - set.insert(index); - set - }); + if self.built { + panic!("Cannot add Args to the map after the map is built"); } + let index = self.value_index.len(); + self.value_index.push(value); + index } //TODO ::push_many([x, y]) pub fn insert_key(&mut self, key: KeyType, index: usize) { - if index >= self.values.len() { + if index >= self.value_index.len() { panic!("Index out of bounds"); } @@ -115,14 +92,7 @@ where } } - pub fn is_empty(&self) -> bool { self.keys.is_empty() && self.values.is_empty() } - - pub fn remove_by_name(&mut self, _name: &str) -> Option { unimplemented!() } - - pub fn remove(&mut self, _key: KeyType) -> Option { unimplemented!() } - //TODO ::remove_many([KeyA, KeyB]) - //? probably shouldn't add a possibility for removal? - //? or remove by replacement by some dummy object, so the order is preserved + pub fn is_empty(&self) -> bool { self.keys.is_empty() && self.value_index.is_empty() } pub fn remove_key(&mut self, key: KeyType) { self.keys.remove(&key); } //TODO ::remove_keys([KeyA, KeyB]) @@ -153,6 +123,30 @@ where } } +fn _get_keys(arg: &Arg) -> Vec { + if let Some(index) = arg.index { + return vec![KeyType::Position(index)]; + } + + let mut keys = vec![]; + if let Some(c) = arg.short { + keys.push(KeyType::Short(c)); + } + if let Some(ref aliases) = arg.aliases { + for long in aliases + .iter() + .map(|(a, _)| KeyType::Long(OsString::from(a))) + { + keys.push(long); + } + } + if let Some(long) = arg.long { + keys.push(KeyType::Long(OsString::from(long))); + } + + keys +} + impl<'a, 'b> MKeyMap> { pub fn insert_key_by_name(&mut self, key: KeyType, name: &str) { let index = self.find_by_name(name); @@ -160,27 +154,14 @@ impl<'a, 'b> MKeyMap> { self.keys.insert(key, index); } - pub fn make_entries(&mut self, arg: Arg<'a, 'b>) -> usize { - let short = arg.short.map(|c| KeyType::Short(c)); - let positional = arg.index.map(|n| KeyType::Position(n)); + pub fn _build(&mut self) { + self.built = true; - let mut longs = arg - .aliases - .clone() - .map(|v| { - v.iter() - .map(|(n, _)| KeyType::Long(OsString::from(n))) - .collect() - }).unwrap_or(Vec::new()); - - longs.extend(arg.long.map(|l| KeyType::Long(OsString::from(l)))); - - let index = self.push(arg); - short.map(|s| self.insert_key(s, index)); - positional.map(|p| self.insert_key(p, index)); - longs.into_iter().map(|l| self.insert_key(l, index)).count(); - - index + for (i, arg) in self.value_index.iter_mut().enumerate() { + for k in _get_keys(arg) { + self.keys.insert(k, i); + } + } } pub fn make_entries_by_index(&mut self, index: usize) { @@ -209,52 +190,49 @@ impl<'a, 'b> MKeyMap> { longs.into_iter().map(|l| self.insert_key(l, index)).count(); } - pub fn mut_arg(&mut self, name: &str, f: F) - where - F: FnOnce(Arg<'a, 'b>) -> Arg<'a, 'b>, - { - let index = self.find_by_name(name); - let new_arg = f(self.value_index[index].clone()); - - let value_key = self - .values - .iter() - .filter(|(_, v)| v.contains(&index)) - .map(|(k, _)| k) - .next() - .map(|&x| x); - value_key.map(|k| { - self.values.entry(k).and_modify(|v| { - v.remove(&index); - }) - }); - - let mut hasher = DefaultHasher::new(); - - new_arg.hash(&mut hasher); - - let hash = hasher.finish(); - self.values - .entry(hash) - .and_modify(|x| { - x.insert(index); - }).or_insert({ - let mut set = HashSet::new(); - set.insert(index); - set - }); - - self.value_index.push(new_arg); - self.value_index.swap_remove(index); - self.make_entries_by_index(index); - } - pub fn find_by_name(&mut self, name: &str) -> usize { self.value_index .iter() .position(|x| x.name == name) .expect("No such name found") } + + pub fn remove(&mut self, key: KeyType) -> Option> { + if self.built { + panic!("Cannot remove args after being built"); + } + let index = if let Some(index) = self.keys.get(&key) { + index.clone() + } else { + return None; + }; + let arg = self.value_index.swap_remove(index); + for key in _get_keys(&arg) { + let _ = self.keys.remove(&key); + } + Some(arg) + } + //TODO ::remove_many([KeyA, KeyB]) + //? probably shouldn't add a possibility for removal? + //? or remove by replacement by some dummy object, so the order is preserved + + pub fn remove_by_name(&mut self, _name: &str) -> Option> { + if self.built { + panic!("Cannot remove args after being built"); + } + let mut index = None; + for (i, arg) in self.value_index.iter().enumerate() { + if arg.name == _name { + index = Some(i); + break; + } + } + if let Some(i) = index { + Some(self.value_index.swap_remove(i)) + } else { + None + } + } } #[derive(Debug)] @@ -368,11 +346,11 @@ mod tests { map.insert(Long(OsString::from("One")), Arg::with_name("Value1")); - let orig_len = map.values.len(); + let orig_len = map.value_index.len(); map.insert(Long(OsString::from("Two")), Arg::with_name("Value1")); - assert_eq!(map.values.len(), orig_len); + assert_eq!(map.value_index.len(), orig_len); assert_eq!( map.get(Long(OsString::from("One"))), map.get(Long(OsString::from("Two"))) @@ -399,7 +377,7 @@ mod tests { map.get(Long(OsString::from("One"))), map.get(Long(OsString::from("Two"))) ); - assert_eq!(map.values.len(), 1); + assert_eq!(map.value_index.len(), 1); } // #[test] @@ -437,7 +415,7 @@ mod tests { map.remove_key(Long(OsString::from("One"))); assert_eq!(map.keys.len(), 1); - assert_eq!(map.values.len(), 1); + assert_eq!(map.value_index.len(), 1); } #[test] diff --git a/tests/help.rs b/tests/help.rs index cadaaa4d..0e8615b4 100644 --- a/tests/help.rs +++ b/tests/help.rs @@ -570,8 +570,7 @@ fn help_subcommand() { SubCommand::with_name("test") .about("tests things") .arg_from_usage("-v --verbose 'with verbosity'"), - ) - .get_matches_from_safe(vec!["myprog", "help"]); + ).get_matches_from_safe(vec!["myprog", "help"]); assert!(m.is_err()); assert_eq!(m.unwrap_err().kind, ErrorKind::HelpDisplayed); @@ -603,35 +602,30 @@ fn args_with_last_usage() { .short('v') .long("verbose") .setting(ArgSettings::MultipleOccurrences), - ) - .arg( + ).arg( Arg::with_name("timeout") .help("Timeout in seconds.") .short('t') .long("timeout") .value_name("SECONDS"), - ) - .arg( + ).arg( Arg::with_name("frequency") .help("The sampling frequency.") .short('f') .long("frequency") .value_name("HERTZ"), - ) - .arg( + ).arg( Arg::with_name("binary path") .help("The path of the binary to be profiled. for a binary.") .value_name("BINFILE"), - ) - .arg( + ).arg( Arg::with_name("pass through args") .help("Any arguments you wish to pass to the being profiled.") .settings(&[ ArgSettings::MultipleValues, ArgSettings::MultipleOccurrences, ArgSettings::Last, - ]) - .value_name("ARGS"), + ]).value_name("ARGS"), ); assert!(test::compare_output( app, @@ -759,8 +753,7 @@ fn issue_626_unicode_cutoff() { beverages. Some coffeehouses also serve cold beverages such as \ iced coffee and iced tea. Many cafés also serve some type of \ food, such as light snacks, muffins, or pastries.", - ) - .takes_value(true), + ).takes_value(true), ); assert!(test::compare_output( app, @@ -782,8 +775,7 @@ fn hide_possible_vals() { .possible_values(&["fast", "slow"]) .help("Some vals") .takes_value(true), - ) - .arg( + ).arg( Arg::with_name("cafe") .short('c') .long("cafe") @@ -936,15 +928,13 @@ fn issue_702_multiple_values() { .short('s') .long("some") .takes_value(true), - ) - .arg( + ).arg( Arg::with_name("other") .help("some other option") .short('o') .long("other") .takes_value(true), - ) - .arg( + ).arg( Arg::with_name("label") .help("a label") .short('l') @@ -963,8 +953,7 @@ fn long_about() { .about("bar") .long_about( "something really really long, with\nmultiple lines of text\nthat should be displayed", - ) - .arg(Arg::with_name("arg1").help("some option")); + ).arg(Arg::with_name("arg1").help("some option")); assert!(test::compare_output(app, "myapp --help", LONG_ABOUT, false)); } @@ -980,8 +969,7 @@ fn issue_760() { .takes_value(true) .multiple(true) .number_of_values(1), - ) - .arg( + ).arg( Arg::with_name("opt") .help("tests options") .short('O') @@ -1013,8 +1001,7 @@ fn ripgrep_usage_using_templates() { rg [OPTIONS] [-e PATTERN | -f FILE ]... [ ...] rg [OPTIONS] --files [ ...] rg [OPTIONS] --type-list", - ) - .template( + ).template( "\ {bin} {version} @@ -1050,8 +1037,7 @@ fn hidden_args() { .args_from_usage( "-f, --flag 'testing flags' -o, --opt [FILE] 'tests options'", - ) - .arg(Arg::with_name("pos").hidden(true)); + ).arg(Arg::with_name("pos").hidden(true)); assert!(test::compare_output(app, "prog --help", HIDDEN_ARGS, false)); } @@ -1063,8 +1049,7 @@ fn args_negate_sc() { .args_from_usage( "-f, --flag 'testing flags' -o, --opt [FILE] 'tests options'", - ) - .arg(Arg::with_name("PATH").help("help")) + ).arg(Arg::with_name("PATH").help("help")) .subcommand(SubCommand::with_name("test")); assert!(test::compare_output( app, @@ -1081,8 +1066,7 @@ fn issue_1046_hidden_scs() { .args_from_usage( "-f, --flag 'testing flags' -o, --opt [FILE] 'tests options'", - ) - .arg(Arg::with_name("PATH").help("some")) + ).arg(Arg::with_name("PATH").help("some")) .subcommand(SubCommand::with_name("test").setting(AppSettings::Hidden)); assert!(test::compare_output( app, @@ -1169,8 +1153,7 @@ fn last_arg_mult_usage_req_with_sc() { .last(true) .required(true) .help("some"), - ) - .subcommand(SubCommand::with_name("test").about("some")); + ).subcommand(SubCommand::with_name("test").about("some")); assert!(test::compare_output( app, "last --help", @@ -1191,8 +1174,7 @@ fn last_arg_mult_usage_with_sc() { .multiple(true) .last(true) .help("some"), - ) - .subcommand(SubCommand::with_name("test").about("some")); + ).subcommand(SubCommand::with_name("test").about("some")); assert!(test::compare_output(app, "last --help", LAST_ARG_SC, false)); } @@ -1231,8 +1213,19 @@ fn issue_1112_setup() -> App<'static, 'static> { .author("Kevin K.") .about("tests stuff") .version("1.3") - .arg(Arg::from("-h, --help 'some help'")) - .subcommand(SubCommand::with_name("foo").arg(Arg::from("-h, --help 'some help'"))) + .arg( + Arg::with_name("help") + .long("help") + .short('h') + .help("some help"), + ).subcommand( + SubCommand::with_name("foo").arg( + Arg::with_name("help") + .short('h') + .long("help") + .help("some help"), + ), + ) } #[test] @@ -1312,8 +1305,7 @@ fn hide_env_vals() { .possible_values(&["fast", "slow"]) .help("Some vals") .takes_value(true), - ) - .arg( + ).arg( Arg::with_name("cafe") .short('c') .long("cafe") @@ -1346,8 +1338,7 @@ fn show_env_vals() { .possible_values(&["fast", "slow"]) .help("Some vals") .takes_value(true), - ) - .arg( + ).arg( Arg::with_name("cafe") .short('c') .long("cafe") @@ -1375,8 +1366,7 @@ fn custom_headers_headers() { Arg::from("-f, --fake 'some help'") .require_delimiter(true) .value_delimiter(":"), - ) - .help_heading("NETWORKING") + ).help_heading("NETWORKING") .arg( Arg::with_name("no-proxy") .short('n') @@ -1423,19 +1413,16 @@ fn multiple_custom_help_headers() { Arg::from("-f, --fake 'some help'") .require_delimiter(true) .value_delimiter(":"), - ) - .help_heading("NETWORKING") + ).help_heading("NETWORKING") .arg( Arg::with_name("no-proxy") .short('n') .long("no-proxy") .help("Do not use system proxy settings"), - ) - .help_heading("SPECIAL") + ).help_heading("SPECIAL") .arg(Arg::from( "-b, --birthday-song 'Change which song is played for birthdays'", - )) - .stop_custom_headings() + )).stop_custom_headings() .arg( Arg::with_name("speed") .long("speed")