perf: vastly reduces the amount of cloning when adding non-global arg…

…s minus when they're added from App::args which is forced to clone

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: Borrow<Arg<'a, 'b>> + 'a>(mut self, a: A) -> Self {
-        self.p.add_arg(a.borrow());
+    pub fn arg<A: Into<Arg<'a, 'b>>>(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
     }

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();

src/args/arg_builder/base.rs

@@ -1,3 +1,4 @@
+
 use args::{ArgSettings, Arg, ArgFlags};
 
 #[derive(Debug, Clone, Default)]

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,15 +28,22 @@ 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),
         }
     }
 }
 
+impl<'a, 'b> From<Arg<'a, 'b>> 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 {

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<Arg<'n, 'e>> 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()),
+        }
     }
 }
 

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() {
             "..."

src/args/arg_builder/switched.rs

@@ -1,3 +1,4 @@
+
 use Arg;
 
 #[derive(Debug)]

src/args/arg_builder/valued.rs

@@ -41,14 +41,19 @@ impl<'n, 'e> Default for Valued<'n, 'e> {
     }
 }
 
-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();
-        if let Some(ref vec) = a.v.val_names {
+impl<'n, 'e> Valued<'n, 'e> {
+    pub fn fill_in(&mut self) {
+        if let Some(ref vec) = self.val_names {
             if vec.len() > 1 {
-                v.num_vals = Some(vec.len() as u64);
+                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();
         if let Some(ref vec) = a.v.val_names {
             if vec.len() > 1 {
                 v.num_vals = Some(vec.len() as u64);