Skip to content

Commit

Permalink
perf: refactors the POSIX override handling to lazy handling
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@kbknapp
kbknapp committed Jan 25, 2018
1 parent e890b64 commit 7673dfc
Show file tree
Hide file tree
Showing 8 changed files with 129 additions and 160 deletions.
1 change: 1 addition & 0 deletions Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
87 changes: 44 additions & 43 deletions src/app/parser.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -390,14 +390,14 @@ where
}

if starts_new_arg {
{
let any_arg = find!(self.app, &self.cache.unwrap_or(""));
matcher.process_arg_overrides(
any_arg,
&mut self.overrides,
&mut self.required,
);
}
// {
// let any_arg = find!(self.app, &self.cache.unwrap_or(""));
// matcher.process_arg_overrides(
// any_arg,
// &mut self.overrides,
// &mut self.required,
// );
// }

if arg_os.starts_with(b"--") {
needs_val_of = self.parse_long_arg(matcher, &arg_os)?;
Expand Down Expand Up @@ -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(""));
matcher.process_arg_overrides(
any_arg,
&mut self.overrides,
&mut self.required,
);
}
// {
// let any_arg = find!(self.app, &self.cache.unwrap_or(""));
// matcher.process_arg_overrides(
// any_arg,
// &mut self.overrides,
// &mut self.required,
// );
// }
self.cache = Some(p.name);
}
let _ = self.add_val_to_arg(p, &arg_os, matcher)?;
Expand Down Expand Up @@ -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(""));
matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
}
// {
// let any_arg = find!(self.app, &self.cache.unwrap_or(""));
// matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
// }

self.remove_overrides(matcher);

Expand Down Expand Up @@ -804,31 +804,32 @@ where
}

fn remove_overrides(&mut self, matcher: &mut ArgMatcher) {
debugln!("Parser::remove_overrides:{:?};", self.overrides);
for &(overr, name) in &self.overrides {
debugln!("Parser::remove_overrides:iter:({},{});", overr, name);
if matcher.is_present(overr) {
debugln!(
"Parser::remove_overrides:iter:({},{}): removing {};",
overr,
name,
name
);
matcher.remove(name);
for i in (0..self.required.len()).rev() {
debugln!(
"Parser::remove_overrides:iter:({},{}): removing required {};",
overr,
name,
name
);
if self.required[i] == name {
self.required.swap_remove(i);
break;
debugln!("Parser::remove_overrides;");
let mut to_rem: Vec<&str> = Vec::new();
let mut seen: Vec<&str> = Vec::new();
for name in matcher.arg_names() {
debugln!("Parser::remove_overrides:iter:{};", name);
if let Some(arg) = find!(self.app, name) {
if let Some(ref overrides) = arg.overrides {
debugln!("Parser::remove_overrides:iter:{}:{:?};", name, overrides);
for o in overrides {
if matcher.is_present(o) && !seen.contains(o) {
debugln!("Parser::remove_overrides:iter:{}:iter:{}: self;", name, o);
to_rem.push(arg.name);
} else {
debugln!("Parser::remove_overrides:iter:{}:iter:{}: other;", name, o);
to_rem.push(o);
}
}
}
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>(
Expand Down
1 change: 0 additions & 1 deletion src/app/usage.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
136 changes: 68 additions & 68 deletions src/app/validator.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 matcher.iter() {
for name in &self.gather_conflicts(matcher) {
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!(
"Validator::validate_blacklist:iter:{}: groups contains it...",
name
);
debugln!("Validator::validate_blacklist:iter:{}:group;", 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) {
Expand All @@ -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() {
Expand Down Expand Up @@ -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| {
Expand Down Expand Up @@ -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)
}
}
Loading

0 comments on commit 7673dfc

Please sign in to comment.