perf: refactors the POSIX override handling to lazy handling

This commit primarily changes to a lazy handling of POSIX overrides by
relying on github.com/bluss/ordermap instead of the old HashMap impl.
The ordermap allows us to keep track of which arguments arrived first,
and therefore determine which ones should be removed when an override
conflict is found.

This has the added benefit of we no longer have to do the bookkeeping to
keep track and override args as they come in, we can do it once at the
end.

Finally, ordermap allows fast Vec like iteration of the keys, which we
end up doing several times. Benching is still TBD once the v3 prep is
done, but this change should have a meaningful impact.

Cargo.toml

@@ -23,6 +23,7 @@ appveyor = { repository = "kbknapp/clap-rs" }
 bitflags              = "1.0"
 unicode-width         = "0.1.4"
 textwrap              = "0.9.0"
+ordermap			  = "0.3.5"
 strsim    = { version = "0.7.0",  optional = true }
 ansi_term = { version = "0.10.0",  optional = true }
 yaml-rust = { version = "0.3.5",  optional = true }

src/app/parser.rs

@@ -47,7 +47,7 @@ where
     pub app: &'c mut App<'a, 'b>,
     pub required: Vec<&'a str>,
     pub r_ifs: Vec<(&'a str, &'b str, &'a str)>,
-    pub overrides: Vec<(&'b str, &'a str)>,
+    pub overriden: Vec<&'a str>,
     cache: Option<&'a str>,
     num_opts: usize,
     num_flags: usize,
@@ -97,7 +97,7 @@ where
             app: app,
             required: reqs,
             r_ifs: Vec::new(),
-            overrides: Vec::new(),
+            overriden: Vec::new(),
             cache: None,
             num_opts: 0,
             num_flags: 0,
@@ -390,14 +390,14 @@ where
                 }
 
                 if starts_new_arg {
-                    {
+                    // {
-                        let any_arg = find!(self.app, &self.cache.unwrap_or(""));
+                    //     let any_arg = find!(self.app, &self.cache.unwrap_or(""));
-                        matcher.process_arg_overrides(
+                    //     matcher.process_arg_overrides(
-                            any_arg,
+                    //         any_arg,
-                            &mut self.overrides,
+                    //         &mut self.overrides,
-                            &mut self.required,
+                    //         &mut self.required,
-                        );
+                    //     );
-                    }
+                    // }
 
                     if arg_os.starts_with(b"--") {
                         needs_val_of = self.parse_long_arg(matcher, &arg_os)?;
@@ -527,14 +527,14 @@ where
                     self.app.settings.set(AS::TrailingValues);
                 }
                 if self.cache.map_or(true, |name| name != p.name) {
-                    {
+                    // {
-                        let any_arg = find!(self.app, &self.cache.unwrap_or(""));
+                    //     let any_arg = find!(self.app, &self.cache.unwrap_or(""));
-                        matcher.process_arg_overrides(
+                    //     matcher.process_arg_overrides(
-                            any_arg,
+                    //         any_arg,
-                            &mut self.overrides,
+                    //         &mut self.overrides,
-                            &mut self.required,
+                    //         &mut self.required,
-                        );
+                    //     );
-                    }
+                    // }
                     self.cache = Some(p.name);
                 }
                 let _ = self.add_val_to_arg(p, &arg_os, matcher)?;
@@ -647,10 +647,10 @@ where
         }
 
         // In case the last arg was new, we  need to process it's overrides
-        {
+        // {
-            let any_arg = find!(self.app, &self.cache.unwrap_or(""));
+        //     let any_arg = find!(self.app, &self.cache.unwrap_or(""));
-            matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
+        //     matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
-        }
+        // }
 
         self.remove_overrides(matcher);
 
@@ -804,31 +804,32 @@ where
     }
 
     fn remove_overrides(&mut self, matcher: &mut ArgMatcher) {
-        debugln!("Parser::remove_overrides:{:?};", self.overrides);
+        debugln!("Parser::remove_overrides;");
-        for &(overr, name) in &self.overrides {
+        let mut to_rem: Vec<&str> = Vec::new();
-            debugln!("Parser::remove_overrides:iter:({},{});", overr, name);
+        let mut seen: Vec<&str> = Vec::new();
-            if matcher.is_present(overr) {
+        for name in matcher.arg_names() {
-                debugln!(
+            debugln!("Parser::remove_overrides:iter:{};", name);
-                    "Parser::remove_overrides:iter:({},{}): removing {};",
+            if let Some(arg) = find!(self.app, name) {
-                    overr,
+                if let Some(ref overrides) = arg.overrides {
-                    name,
+                    debugln!("Parser::remove_overrides:iter:{}:{:?};", name, overrides);
-                    name
+                    for o in overrides {
-                );
+                        if matcher.is_present(o) && !seen.contains(o) {
-                matcher.remove(name);
+                            debugln!("Parser::remove_overrides:iter:{}:iter:{}: self;", name, o);
-                for i in (0..self.required.len()).rev() {
+                            to_rem.push(arg.name);
-                    debugln!(
+                        } else {
-                        "Parser::remove_overrides:iter:({},{}): removing required {};",
+                            debugln!("Parser::remove_overrides:iter:{}:iter:{}: other;", name, o);
-                        overr,
+                            to_rem.push(o);
-                        name,
+                        }
-                        name
-                    );
-                    if self.required[i] == name {
-                        self.required.swap_remove(i);
-                        break;
                     }
                 }
+                seen.push(arg.name);
             }
         }
+        for name in &to_rem {
+            debugln!("Parser::remove_overrides:iter:{}: removing;", name);
+            matcher.remove(name);
+            self.overriden.push(name);
+        }
     }
 
     fn parse_subcommand<I, T>(

src/app/usage.rs

@@ -31,7 +31,6 @@ impl<'a, 'b, 'c, 'z> Usage<'a, 'b, 'c, 'z> {
     pub fn create_error_usage(&self, matcher: &ArgMatcher<'a>, extra: Option<&str>) -> String {
         let mut args: Vec<_> = matcher
             .arg_names()
-            .iter()
             .filter(|ref n| {
                 if let Some(a) = find!(self.0.app, **n) {
                     !a.is_set(ArgSettings::Required) && !a.is_set(ArgSettings::Hidden)

src/app/validator.rs

@@ -146,7 +146,7 @@ impl<'a, 'b, 'c, 'z> Validator<'a, 'b, 'c, 'z> {
         Ok(())
     }
 
-    fn build_err(&self, name: &str, matcher: &ArgMatcher<'a>) -> ClapResult<()> {
+    fn build_conflict_err(&self, name: &str, matcher: &ArgMatcher<'a>) -> ClapResult<()> {
         debugln!("build_err!: name={}", name);
         let mut c_with = find_from!(self.0.app, &name, blacklist, &matcher);
         c_with = c_with.or(find!(self.0.app, &name)
@@ -172,78 +172,24 @@ impl<'a, 'b, 'c, 'z> Validator<'a, 'b, 'c, 'z> {
 
     fn validate_blacklist(&self, matcher: &mut ArgMatcher<'a>) -> ClapResult<()> {
         debugln!("Validator::validate_blacklist;");
-        let mut conflicts: Vec<&str> = vec![];
+        for name in &self.gather_conflicts(matcher) {
-        for (&name, _) in matcher.iter() {
             debugln!("Validator::validate_blacklist:iter:{};", name);
-            if let Some(grps) = self.0.groups_for_arg(name) {
-                for grp in &grps {
-                    if let Some(g) = self.0.app.groups.iter().find(|g| &g.name == grp) {
-                        if !g.multiple {
-                            for arg in &g.args {
-                                if arg == &name {
-                                    continue;
-                                }
-                                conflicts.push(arg);
-                            }
-                        }
-                        if let Some(ref gc) = g.conflicts {
-                            conflicts.extend(&*gc);
-                        }
-                    }
-                }
-            }
-            if let Some(arg) = find!(self.0.app, &name) {
-                if let Some(ref bl) = arg.blacklist {
-                    for conf in bl {
-                        if matcher.get(conf).is_some() {
-                            conflicts.push(conf);
-                        }
-                    }
-                }
-            } else {
-                debugln!("Validator::validate_blacklist:iter:{}:group;", name);
-                let args = self.0.arg_names_in_group(name);
-                for arg in &args {
-                    debugln!(
-                        "Validator::validate_blacklist:iter:{}:group:iter:{};",
-                        name,
-                        arg
-                    );
-                    if let Some(ref bl) = find!(self.0.app, arg).unwrap().blacklist {
-                        for conf in bl {
-                            if matcher.get(conf).is_some() {
-                                conflicts.push(conf);
-                            }
-                        }
-                    }
-                }
-            }
-        }
-
-        for name in &conflicts {
-            debugln!(
-                "Validator::validate_blacklist:iter:{}: Checking blacklisted arg",
-                name
-            );
             let mut should_err = false;
             if groups!(self.0.app).any(|g| &g.name == name) {
-                debugln!(
+                debugln!("Validator::validate_blacklist:iter:{}:group;", name);
-                    "Validator::validate_blacklist:iter:{}: groups contains it...",
-                    name
-                );
                 for n in self.0.arg_names_in_group(name) {
                     debugln!(
-                        "Validator::validate_blacklist:iter:{}:iter:{}: looking in group...",
+                        "Validator::validate_blacklist:iter:{}:group:iter:{};",
                         name,
                         n
                     );
                     if matcher.contains(n) {
                         debugln!(
-                            "Validator::validate_blacklist:iter:{}:iter:{}: matcher contains it...",
+                            "Validator::validate_blacklist:iter:{}:group:iter:{}: found;",
                             name,
                             n
                         );
-                        return self.build_err(n, matcher);
+                        return self.build_conflict_err(n, matcher);
                     }
                 }
             } else if let Some(ma) = matcher.get(name) {
@@ -254,12 +200,66 @@ impl<'a, 'b, 'c, 'z> Validator<'a, 'b, 'c, 'z> {
                 should_err = ma.occurs > 0;
             }
             if should_err {
-                return self.build_err(*name, matcher);
+                return self.build_conflict_err(*name, matcher);
             }
         }
         Ok(())
     }
 
+    fn gather_conflicts(&self, matcher: &mut ArgMatcher<'a>) -> Vec<&'a str> {
+        debugln!("Validator::gather_conflicts;");
+        let mut conflicts = vec![];
+        for name in matcher.arg_names() {
+            debugln!("Validator::gather_conflicts:iter:{};", name);
+            if let Some(arg) = find!(self.0.app, name) {
+                if let Some(ref bl) = arg.blacklist {
+                    for conf in bl {
+                        if matcher.get(conf).is_some() {
+                            conflicts.push(*conf);
+                        }
+                    }
+                }
+                if let Some(grps) = self.0.groups_for_arg(name) {
+                    for grp in &grps {
+                        if let Some(g) = find!(self.0.app, grp, groups) {
+                            if !g.multiple {
+                                for g_arg in &g.args {
+                                    if &g_arg == &name {
+                                        continue;
+                                    }
+                                    conflicts.push(g_arg);
+                                }
+                            }
+                            if let Some(ref gc) = g.conflicts {
+                                conflicts.extend(&*gc);
+                            }
+                        }
+                    }
+                }
+            } else {
+                debugln!("Validator::gather_conflicts:iter:{}:group;", name);
+                let args = self.0.arg_names_in_group(name);
+                for arg in &args {
+                    debugln!(
+                        "Validator::gather_conflicts:iter:{}:group:iter:{};",
+                        name,
+                        arg
+                    );
+                    if let Some(ref bl) =
+                        find!(self.0.app, arg).expect(INTERNAL_ERROR_MSG).blacklist
+                    {
+                        for conf in bl {
+                            if matcher.get(conf).is_some() {
+                                conflicts.push(conf);
+                            }
+                        }
+                    }
+                }
+            }
+        }
+        conflicts
+    }
+
     fn validate_matched_args(&self, matcher: &mut ArgMatcher<'a>) -> ClapResult<()> {
         debugln!("Validator::validate_matched_args;");
         for (name, ma) in matcher.iter() {
@@ -438,6 +438,13 @@ impl<'a, 'b, 'c, 'z> Validator<'a, 'b, 'c, 'z> {
         Ok(())
     }
 
+    fn is_missing_required_ok(&self, a: &Arg<'a, 'b>, matcher: &ArgMatcher<'a>) -> bool {
+        debugln!("Validator::is_missing_required_ok: a={}", a.name);
+        self.validate_arg_conflicts(a, matcher).unwrap_or(false)
+            || self.validate_required_unless(a, matcher).unwrap_or(false)
+            || self.0.overriden.contains(&a.name)
+    }
+
     fn validate_arg_conflicts(&self, a: &Arg<'a, 'b>, matcher: &ArgMatcher<'a>) -> Option<bool> {
         debugln!("Validator::validate_arg_conflicts: a={:?};", a.name);
         a.blacklist.as_ref().map(|bl| {
@@ -506,11 +513,4 @@ impl<'a, 'b, 'c, 'z> Validator<'a, 'b, 'c, 'z> {
             self.0.app.color(),
         ))
     }
-
-    #[inline]
-    fn is_missing_required_ok(&self, a: &Arg<'a, 'b>, matcher: &ArgMatcher<'a>) -> bool {
-        debugln!("Validator::is_missing_required_ok: a={}", a.name);
-        self.validate_arg_conflicts(a, matcher).unwrap_or(false)
-            || self.validate_required_unless(a, matcher).unwrap_or(false)
-    }
 }

src/args/arg_matcher.rs

@@ -1,10 +1,11 @@
 // Std
-use std::collections::hash_map::{Entry, Iter};
-use std::collections::HashMap;
 use std::ffi::OsStr;
-use std::ops::Deref;
+use std::collections::HashMap;
 use std::mem;
 
+// Third Party
+use ordermap;
+
 // Internal
 use args::{Arg, ArgMatches, MatchedArg, SubCommand};
 use args::settings::ArgSettings;
@@ -20,44 +21,6 @@ impl<'a> Default for ArgMatcher<'a> {
 impl<'a> ArgMatcher<'a> {
     pub fn new() -> Self { ArgMatcher::default() }
 
-    pub fn process_arg_overrides<'b>(
-        &mut self,
-        a: Option<&Arg<'a, 'b>>,
-        overrides: &mut Vec<(&'b str, &'a str)>,
-        required: &mut Vec<&'a str>,
-    ) {
-        debugln!(
-            "ArgMatcher::process_arg_overrides:{:?};",
-            a.map_or(None, |a| Some(a.name))
-        );
-        if let Some(aa) = a {
-            if let Some(ref a_overrides) = aa.overrides {
-                for overr in a_overrides {
-                    debugln!("ArgMatcher::process_arg_overrides:iter:{};", overr);
-                    if self.is_present(overr) {
-                        debugln!(
-                            "ArgMatcher::process_arg_overrides:iter:{}: removing from matches;",
-                            overr
-                        );
-                        self.remove(overr);
-                        for i in (0..required.len()).rev() {
-                            if &required[i] == overr {
-                                debugln!(
-                                    "ArgMatcher::process_arg_overrides:iter:{}: removing required;",
-                                    overr
-                                );
-                                required.swap_remove(i);
-                                break;
-                            }
-                        }
-                    } else {
-                        overrides.push((overr, aa.name));
-                    }
-                }
-            }
-        }
-    }
-
     pub fn is_present(&self, name: &str) -> bool { self.0.is_present(name) }
 
     pub fn propagate_globals(&mut self, global_arg_vec: &[&'a str]) {
@@ -126,15 +89,17 @@ impl<'a> ArgMatcher<'a> {
 
     pub fn usage(&mut self, usage: String) { self.0.usage = Some(usage); }
 
-    pub fn arg_names(&'a self) -> Vec<&'a str> { self.0.args.keys().map(Deref::deref).collect() }
+    pub fn arg_names(&'a self) -> ordermap::Keys<&'a str, MatchedArg> { self.0.args.keys() }
 
-    pub fn entry(&mut self, arg: &'a str) -> Entry<&'a str, MatchedArg> { self.0.args.entry(arg) }
+    pub fn entry(&mut self, arg: &'a str) -> ordermap::Entry<&'a str, MatchedArg> {
+        self.0.args.entry(arg)
+    }
 
     pub fn subcommand(&mut self, sc: SubCommand<'a>) { self.0.subcommand = Some(Box::new(sc)); }
 
     pub fn subcommand_name(&self) -> Option<&str> { self.0.subcommand_name() }
 
-    pub fn iter(&self) -> Iter<&str, MatchedArg> { self.0.args.iter() }
+    pub fn iter(&self) -> ordermap::Iter<&str, MatchedArg> { self.0.args.iter() }
 
     pub fn inc_occurrence_of(&mut self, arg: &'a str) {
         debugln!("ArgMatcher::inc_occurrence_of: arg={}", arg);

src/args/arg_matches.rs

@@ -1,10 +1,12 @@
 // Std
 use std::borrow::Cow;
-use std::collections::HashMap;
 use std::ffi::{OsStr, OsString};
 use std::iter::Map;
 use std::slice::Iter;
 
+// Third Party
+use ordermap::OrderMap;
+
 // Internal
 use INVALID_UTF8;
 use args::MatchedArg;
@@ -60,7 +62,7 @@ use args::SubCommand;
 #[derive(Debug, Clone)]
 pub struct ArgMatches<'a> {
     #[doc(hidden)]
-    pub args: HashMap<&'a str, MatchedArg>,
+    pub args: OrderMap<&'a str, MatchedArg>,
     #[doc(hidden)]
     pub subcommand: Option<Box<SubCommand<'a>>>,
     #[doc(hidden)]
@@ -70,7 +72,7 @@ pub struct ArgMatches<'a> {
 impl<'a> Default for ArgMatches<'a> {
     fn default() -> Self {
         ArgMatches {
-            args: HashMap::new(),
+            args: OrderMap::new(),
             subcommand: None,
             usage: None,
         }

src/lib.rs

@@ -534,6 +534,7 @@ extern crate ansi_term;
 extern crate atty;
 #[macro_use]
 extern crate bitflags;
+extern crate ordermap;
 #[cfg(feature = "suggestions")]
 extern crate strsim;
 #[cfg(feature = "wrap_help")]

src/macros.rs

@@ -955,7 +955,7 @@ macro_rules! find_from {
     ($app:expr, $arg_name:expr, $from:ident, $matcher:expr) => {{
         let mut ret = None;
         for k in $matcher.arg_names() {
-            if let Some(a) = find!($app, &k) {
+            if let Some(a) = find!($app, k) {
                 if let Some(ref v) = a.$from {
                     if v.contains($arg_name) {
                         ret = Some(a.to_string());