From 8da0303bc02db5fe047cfc0631a9da41d9dc60f7 Mon Sep 17 00:00:00 2001 From: Kevin K Date: Mon, 27 Feb 2017 23:37:25 -0500 Subject: [PATCH] perf: vastly reduces the amount of cloning when adding non-global args minus when they're added from App::args which is forced to clone --- src/app/mod.rs | 11 ++-- src/app/parser.rs | 86 +++++++++++++++++++++++++----- src/args/arg_builder/base.rs | 1 + src/args/arg_builder/flag.rs | 12 ++++- src/args/arg_builder/option.rs | 28 +++++----- src/args/arg_builder/positional.rs | 26 ++++----- src/args/arg_builder/switched.rs | 1 + src/args/arg_builder/valued.rs | 15 ++++-- 8 files changed, 129 insertions(+), 51 deletions(-) diff --git a/src/app/mod.rs b/src/app/mod.rs index f53da523..67683f0e 100644 --- a/src/app/mod.rs +++ b/src/app/mod.rs @@ -6,7 +6,6 @@ mod meta; mod help; // Std -use std::borrow::Borrow; use std::env; use std::ffi::{OsStr, OsString}; use std::fmt; @@ -639,8 +638,8 @@ impl<'a, 'b> App<'a, 'b> { /// # ; /// ``` /// [argument]: ./struct.Arg.html - pub fn arg> + 'a>(mut self, a: A) -> Self { - self.p.add_arg(a.borrow()); + pub fn arg>>(mut self, a: A) -> Self { + self.p.add_arg(a.into()); self } @@ -660,7 +659,7 @@ impl<'a, 'b> App<'a, 'b> { /// [arguments]: ./struct.Arg.html pub fn args(mut self, args: &[Arg<'a, 'b>]) -> Self { for arg in args { - self.p.add_arg(arg); + self.p.add_arg_ref(arg); } self } @@ -683,7 +682,7 @@ impl<'a, 'b> App<'a, 'b> { /// [`Arg`]: ./struct.Arg.html /// [`Arg::from_usage`]: ./struct.Arg.html#method.from_usage pub fn arg_from_usage(mut self, usage: &'a str) -> Self { - self.p.add_arg(&Arg::from_usage(usage)); + self.p.add_arg(Arg::from_usage(usage)); self } @@ -715,7 +714,7 @@ impl<'a, 'b> App<'a, 'b> { if l.is_empty() { continue; } - self.p.add_arg(&Arg::from_usage(l)); + self.p.add_arg(Arg::from_usage(l)); } self } diff --git a/src/app/parser.rs b/src/app/parser.rs index 6f4105c1..9253b5df 100644 --- a/src/app/parser.rs +++ b/src/app/parser.rs @@ -110,8 +110,8 @@ impl<'a, 'b> Parser<'a, 'b> self.gen_completions_to(for_shell, &mut file) } - // actually adds the arguments - pub fn add_arg(&mut self, a: &Arg<'a, 'b>) { + #[inline] + fn debug_asserts(&self, a: &Arg) { debug_assert!(!arg_names!(self).any(|name| name == a.b.name), format!("Non-unique argument name: {} is already in use", a.b.name)); if let Some(l) = a.s.long { @@ -124,11 +124,33 @@ impl<'a, 'b> Parser<'a, 'b> format!("Argument short must be unique\n\n\t-{} is already in use", s)); } + let i = if a.index.is_none() { + (self.positionals.len() + 1) + } else { + a.index.unwrap() as usize + }; + debug_assert!(!self.positionals.contains_key(i), + format!("Argument \"{}\" has the same index as another positional \ + argument\n\n\tPerhaps try .multiple(true) to allow one positional argument \ + to take multiple values", + a.b.name)); + debug_assert!(!(a.is_set(ArgSettings::Required) && a.is_set(ArgSettings::Global)), + format!("Global arguments cannot be required.\n\n\t'{}' is marked as \ + global and required", + a.b.name)); + } + + #[inline] + fn add_conditional_reqs(&mut self, a: &Arg<'a, 'b>) { if let Some(ref r_ifs) = a.r_ifs { for &(arg, val) in r_ifs { self.r_ifs.push((arg, val, a.b.name)); } } + } + + #[inline] + fn add_arg_groups(&mut self, a: &Arg<'a, 'b>) { if let Some(ref grps) = a.b.groups { for g in grps { let mut found = false; @@ -143,24 +165,64 @@ impl<'a, 'b> Parser<'a, 'b> } } } + } + + #[inline] + fn add_reqs(&mut self, a: &Arg<'a, 'b>) { if a.is_set(ArgSettings::Required) { + // If the arg is required, add all it's requirements to master required list + if let Some(ref areqs) = a.b.requires { + for name in areqs.iter().filter(|&&(val, _)| val.is_none()).map(|&(_, name)| name) { + self.required.push(name); + } + } self.required.push(a.b.name); } + } + + // actually adds the arguments + pub fn add_arg(&mut self, a: Arg<'a, 'b>) { + // if it's global we have to clone anyways + if a.is_set(ArgSettings::Global) { + return self.add_arg_ref(&a); + } + self.debug_asserts(&a); + self.add_conditional_reqs(&a); + self.add_arg_groups(&a); + self.add_reqs(&a); if a.index.is_some() || (a.s.short.is_none() && a.s.long.is_none()) { let i = if a.index.is_none() { (self.positionals.len() + 1) } else { a.index.unwrap() as usize }; - debug_assert!(!self.positionals.contains_key(i), - format!("Argument \"{}\" has the same index as another positional \ - argument\n\n\tPerhaps try .multiple(true) to allow one positional argument \ - to take multiple values", - a.b.name)); - let pb = PosBuilder::from_arg(a, i as u64, &mut self.required); + self.positionals.insert(i, PosBuilder::from_arg(a, i as u64)); + } else if a.is_set(ArgSettings::TakesValue) { + let mut ob = OptBuilder::from(a); + ob.s.unified_ord = self.flags.len() + self.opts.len(); + self.opts.push(ob); + } else { + let mut fb = FlagBuilder::from(a); + fb.s.unified_ord = self.flags.len() + self.opts.len(); + self.flags.push(fb); + } + } + // actually adds the arguments but from a borrow (which means we have to do some clonine) + pub fn add_arg_ref(&mut self, a: &Arg<'a, 'b>) { + self.debug_asserts(&a); + self.add_conditional_reqs(&a); + self.add_arg_groups(&a); + self.add_reqs(&a); + if a.index.is_some() || (a.s.short.is_none() && a.s.long.is_none()) { + let i = if a.index.is_none() { + (self.positionals.len() + 1) + } else { + a.index.unwrap() as usize + }; + let pb = PosBuilder::from_arg_ref(a, i as u64); self.positionals.insert(i, pb); } else if a.is_set(ArgSettings::TakesValue) { - let mut ob = OptBuilder::from_arg(a, &mut self.required); + let mut ob = OptBuilder::from(a); ob.s.unified_ord = self.flags.len() + self.opts.len(); self.opts.push(ob); } else { @@ -169,10 +231,6 @@ impl<'a, 'b> Parser<'a, 'b> self.flags.push(fb); } if a.is_set(ArgSettings::Global) { - debug_assert!(!a.is_set(ArgSettings::Required), - format!("Global arguments cannot be required.\n\n\t'{}' is marked as \ - global and required", - a.b.name)); self.global_args.push(a.into()); } } @@ -589,7 +647,7 @@ impl<'a, 'b> Parser<'a, 'b> // done and to recursively call this method { for a in &self.global_args { - sc.p.add_arg(a); + sc.p.add_arg_ref(a); } } sc.p.propogate_globals(); diff --git a/src/args/arg_builder/base.rs b/src/args/arg_builder/base.rs index 95ebe663..1b5e534c 100644 --- a/src/args/arg_builder/base.rs +++ b/src/args/arg_builder/base.rs @@ -1,3 +1,4 @@ + use args::{ArgSettings, Arg, ArgFlags}; #[derive(Debug, Clone, Default)] diff --git a/src/args/arg_builder/flag.rs b/src/args/arg_builder/flag.rs index eb4f5542..6dc37243 100644 --- a/src/args/arg_builder/flag.rs +++ b/src/args/arg_builder/flag.rs @@ -4,6 +4,7 @@ use std::fmt::{Display, Formatter, Result}; use std::rc::Rc; use std::result::Result as StdResult; use std::ffi::{OsStr, OsString}; +use std::mem; // Third Party use vec_map::{self, VecMap}; @@ -27,8 +28,6 @@ impl<'n, 'e> FlagBuilder<'n, 'e> { impl<'a, 'b, 'z> From<&'z Arg<'a, 'b>> for FlagBuilder<'a, 'b> { fn from(a: &'z Arg<'a, 'b>) -> Self { - // No need to check for index() or takes_value() as that is handled above - FlagBuilder { b: Base::from(a), s: Switched::from(a), @@ -36,6 +35,15 @@ impl<'a, 'b, 'z> From<&'z Arg<'a, 'b>> for FlagBuilder<'a, 'b> { } } +impl<'a, 'b> From> for FlagBuilder<'a, 'b> { + fn from(mut a: Arg<'a, 'b>) -> Self { + FlagBuilder { + b: mem::replace(&mut a.b, Base::default()), + s: mem::replace(&mut a.s, Switched::default()), + } + } +} + impl<'n, 'e> Display for FlagBuilder<'n, 'e> { fn fmt(&self, f: &mut Formatter) -> Result { if let Some(l) = self.s.long { diff --git a/src/args/arg_builder/option.rs b/src/args/arg_builder/option.rs index 0cf37931..75f14a73 100644 --- a/src/args/arg_builder/option.rs +++ b/src/args/arg_builder/option.rs @@ -3,6 +3,7 @@ use std::fmt::{Display, Formatter, Result}; use std::rc::Rc; use std::result::Result as StdResult; use std::ffi::{OsStr, OsString}; +use std::mem; // Third Party use vec_map::{self, VecMap}; @@ -23,23 +24,26 @@ pub struct OptBuilder<'n, 'e> impl<'n, 'e> OptBuilder<'n, 'e> { pub fn new(name: &'n str) -> Self { OptBuilder { b: Base::new(name), ..Default::default() } } +} - pub fn from_arg(a: &Arg<'n, 'e>, reqs: &mut Vec<&'n str>) -> Self { - // No need to check for .index() as that is handled above - let ob = OptBuilder { +impl<'n, 'e, 'z> From<&'z Arg<'n, 'e>> for OptBuilder<'n, 'e> { + fn from(a: &'z Arg<'n, 'e>) -> Self { + OptBuilder { b: Base::from(a), s: Switched::from(a), v: Valued::from(a), - }; - // If the arg is required, add all it's requirements to master required list - if a.is_set(ArgSettings::Required) { - if let Some(ref areqs) = a.b.requires { - for r in areqs.iter().filter(|r| r.0.is_none()) { - reqs.push(r.1); - } - } } - ob + } +} + +impl<'n, 'e> From> for OptBuilder<'n, 'e> { + fn from(mut a: Arg<'n, 'e>) -> Self { + a.v.fill_in(); + OptBuilder { + b: mem::replace(&mut a.b, Base::default()), + s: mem::replace(&mut a.s, Switched::default()), + v: mem::replace(&mut a.v, Valued::default()), + } } } diff --git a/src/args/arg_builder/positional.rs b/src/args/arg_builder/positional.rs index 6d9bdf65..73fde4c1 100644 --- a/src/args/arg_builder/positional.rs +++ b/src/args/arg_builder/positional.rs @@ -4,6 +4,7 @@ use std::fmt::{Display, Formatter, Result}; use std::rc::Rc; use std::result::Result as StdResult; use std::ffi::{OsStr, OsString}; +use std::mem; // Third Party use vec_map::{self, VecMap}; @@ -32,10 +33,7 @@ impl<'n, 'e> PosBuilder<'n, 'e> { } } - pub fn from_arg(a: &Arg<'n, 'e>, idx: u64, reqs: &mut Vec<&'n str>) -> Self { - // Create the Positional Argument Builder with each HashSet = None to only - // allocate - // those that require it + pub fn from_arg_ref(a: &Arg<'n, 'e>, idx: u64) -> Self { let mut pb = PosBuilder { b: Base::from(a), v: Valued::from(a), @@ -45,17 +43,21 @@ impl<'n, 'e> PosBuilder<'n, 'e> { (a.v.num_vals.is_some() && a.v.num_vals.unwrap() > 1) { pb.b.settings.set(ArgSettings::Multiple); } - // If the arg is required, add all it's requirements to master required list - if a.is_set(ArgSettings::Required) { - if let Some(ref areqs) = a.b.requires { - for name in areqs.iter().filter(|&&(val, _)| val.is_none()).map(|&(_, name)| name) { - reqs.push(name); - } - } - } pb } + pub fn from_arg(mut a: Arg<'n, 'e>, idx: u64) -> Self { + if a.v.max_vals.is_some() || a.v.min_vals.is_some() || + (a.v.num_vals.is_some() && a.v.num_vals.unwrap() > 1) { + a.b.settings.set(ArgSettings::Multiple); + } + PosBuilder { + b: mem::replace(&mut a.b, Base::default()), + v: mem::replace(&mut a.v, Valued::default()), + index: idx, + } + } + pub fn multiple_str(&self) -> &str { if self.b.settings.is_set(ArgSettings::Multiple) && self.v.val_names.is_none() { "..." diff --git a/src/args/arg_builder/switched.rs b/src/args/arg_builder/switched.rs index 224b2f2b..e42586c2 100644 --- a/src/args/arg_builder/switched.rs +++ b/src/args/arg_builder/switched.rs @@ -1,3 +1,4 @@ + use Arg; #[derive(Debug)] diff --git a/src/args/arg_builder/valued.rs b/src/args/arg_builder/valued.rs index d98e3c34..93d63cce 100644 --- a/src/args/arg_builder/valued.rs +++ b/src/args/arg_builder/valued.rs @@ -41,6 +41,16 @@ impl<'n, 'e> Default for Valued<'n, 'e> { } } +impl<'n, 'e> Valued<'n, 'e> { + pub fn fill_in(&mut self) { + if let Some(ref vec) = self.val_names { + if vec.len() > 1 { + self.num_vals = Some(vec.len() as u64); + } + } + } +} + impl<'n, 'e, 'z> From<&'z Arg<'n, 'e>> for Valued<'n, 'e> { fn from(a: &'z Arg<'n, 'e>) -> Self { let mut v = a.v.clone(); @@ -49,11 +59,6 @@ impl<'n, 'e, 'z> From<&'z Arg<'n, 'e>> for Valued<'n, 'e> { v.num_vals = Some(vec.len() as u64); } } - if let Some(ref vec) = a.v.val_names { - if vec.len() > 1 { - v.num_vals = Some(vec.len() as u64); - } - } v } }