From a75b67838e67773c4855e881afc8e9ad0a290162 Mon Sep 17 00:00:00 2001 From: Kevin K Date: Sun, 29 Jul 2018 16:16:42 -0400 Subject: [PATCH] refactor(Suggestions): changes from string slice to owned string for return values The return value is only used in the error case, and only a single String allocation, therefore the complexities introduced by using string slices isn't worth it. clap primarily only needs to be as fast as possible in the happy path, the error path can afford additional allocations. --- src/mkeymap.rs | 26 +++++----- src/parse/features/suggestions.rs | 49 +++++++++---------- src/parse/parser.rs | 80 ++++++++++++++++++++----------- 3 files changed, 90 insertions(+), 65 deletions(-) diff --git a/src/mkeymap.rs b/src/mkeymap.rs index e2b520f8..01253c8a 100644 --- a/src/mkeymap.rs +++ b/src/mkeymap.rs @@ -1,5 +1,3 @@ -#![feature(nll)] - use build::Arg; use std::collections::hash_map; use std::collections::hash_map::DefaultHasher; @@ -10,8 +8,7 @@ use std::slice; // ! rustdoc #[derive(Default, PartialEq, Debug, Clone)] -pub struct MKeyMap -{ +pub struct MKeyMap { keys: HashMap, value_index: Vec, values: HashMap>, @@ -24,8 +21,9 @@ pub enum KeyType { Position(usize), } -impl MKeyMap -where T: Sized + Hash + PartialEq + Default + Eq +impl MKeyMap +where + T: Sized + Hash + PartialEq + Default + Eq, { pub fn new() -> Self { MKeyMap::default() } //TODO ::from(x), ::with_capacity(n) etc @@ -124,15 +122,13 @@ where T: Sized + Hash + PartialEq + Default + Eq } } - pub fn values(&self) -> Values - { + pub fn values(&self) -> Values { Values { iter: self.value_index.iter(), } } - pub fn values_mut(&mut self) -> ValuesMut - { + pub fn values_mut(&mut self) -> ValuesMut { ValuesMut { iter: self.value_index.iter_mut(), } @@ -146,6 +142,7 @@ where T: Sized + Hash + PartialEq + Default + Eq } } +#[derive(Debug)] pub struct Keys<'a, V: 'a> { iter: hash_map::Keys<'a, KeyType, V>, } @@ -156,6 +153,7 @@ impl<'a, V> Iterator for Keys<'a, V> { fn next(&mut self) -> Option { self.iter.next() } } +#[derive(Debug)] pub struct Values<'a, V: 'a> { iter: slice::Iter<'a, V>, } @@ -166,6 +164,7 @@ impl<'a, V> Iterator for Values<'a, V> { fn next(&mut self) -> Option { self.iter.next() } } +#[derive(Debug)] pub struct ValuesMut<'a, V: 'a> { iter: slice::IterMut<'a, V>, } @@ -176,15 +175,18 @@ impl<'a, V> Iterator for ValuesMut<'a, V> { fn next(&mut self) -> Option { self.iter.next() } } +#[derive(Debug)] pub struct Iter<'c, T> -where T: 'c +where + T: 'c, { map: &'c MKeyMap, keys: Keys<'c, usize>, } impl<'c, T> Iterator for Iter<'c, T> -where T: 'c + Sized + Hash + PartialEq + Default + Eq +where + T: 'c + Sized + Hash + PartialEq + Default + Eq, { type Item = (&'c KeyType, &'c T); diff --git a/src/parse/features/suggestions.rs b/src/parse/features/suggestions.rs index e4b136f6..4bad990c 100644 --- a/src/parse/features/suggestions.rs +++ b/src/parse/features/suggestions.rs @@ -1,4 +1,3 @@ -#![feature(nll)] // Third Party #[cfg(feature = "suggestions")] use strsim; @@ -13,57 +12,59 @@ use output::fmt::Format; /// `Some("foo")`, whereas "blark" would yield `None`. #[cfg(feature = "suggestions")] #[cfg_attr(feature = "lints", allow(needless_lifetimes))] -pub fn did_you_mean<'a, T: ?Sized, I>(v: &str, possible_values: I) -> Option<&'a str> +pub fn did_you_mean(v: &str, possible_values: I) -> Option where - T: AsRef + 'a, - I: IntoIterator, + T: AsRef, + I: IntoIterator, { - let mut candidate: Option<(f64, &str)> = None; + let mut candidate: Option<(f64, String)> = None; for pv in possible_values { let confidence = strsim::jaro_winkler(v, pv.as_ref()); if confidence > 0.8 && (candidate.is_none() || (candidate.as_ref().unwrap().0 < confidence)) { - candidate = Some((confidence, pv.as_ref())); + candidate = Some((confidence, pv.as_ref().to_owned())); } } match candidate { None => None, - Some((_, candidate)) => Some(candidate), + Some((_, candidate)) => Some(candidate.to_owned()), } } #[cfg(not(feature = "suggestions"))] -pub fn did_you_mean<'a, T: ?Sized, I>(_: &str, _: I) -> Option<&'a str> +pub fn did_you_mean(_: &str, _: I) -> Option where - T: AsRef + 'a, - I: IntoIterator, + T: AsRef, + I: IntoIterator, { None } /// Returns a suffix that can be empty, or is the standard 'did you mean' phrase #[cfg_attr(feature = "lints", allow(needless_lifetimes))] -pub fn did_you_mean_flag_suffix<'z, I>( +pub fn did_you_mean_flag_suffix( arg: &str, longs: I, - subcommands: &'z [App], -) -> (String, Option<&'z str>) + subcommands: &[App], +) -> (String, Option) where - I: IntoIterator, + T: AsRef, + I: IntoIterator, { match did_you_mean(arg, longs) { - Some(candidate) => { + Some(ref candidate) => { let suffix = format!( "\n\tDid you mean {}{}?", Format::Good("--"), Format::Good(candidate) ); - return (suffix, Some(candidate)); + return (suffix, Some(candidate.to_owned())); } None => for subcommand in subcommands { - let longs = longs!(subcommand).map(|x| x.to_string_lossy().into_owned()).collect::>(); - - if let Some(candidate) = did_you_mean(arg, longs.iter()) { + if let Some(ref candidate) = did_you_mean( + arg, + longs!(subcommand).map(|x| x.to_string_lossy().into_owned()), + ) { let suffix = format!( "\n\tDid you mean to put '{}{}' after the subcommand '{}'?", Format::Good("--"), @@ -78,15 +79,15 @@ where } /// Returns a suffix that can be empty, or is the standard 'did you mean' phrase -pub fn did_you_mean_value_suffix<'z, T, I>(arg: &str, values: I) -> (String, Option<&'z str>) +pub fn did_you_mean_value_suffix(arg: &str, values: I) -> (String, Option) where - T: AsRef + 'z, - I: IntoIterator, + T: AsRef, + I: IntoIterator, { match did_you_mean(arg, values) { - Some(candidate) => { + Some(ref candidate) => { let suffix = format!("\n\tDid you mean '{}'?", Format::Good(candidate)); - (suffix, Some(candidate)) + (suffix, Some(candidate.to_owned())) } None => (String::new(), None), } diff --git a/src/parse/parser.rs b/src/parse/parser.rs index a200d52a..4bf64f1d 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -38,7 +38,6 @@ use parse::{ArgMatcher, SubCommand}; use util::{ChildGraph, OsStrExt2}; use INVALID_UTF8; use INTERNAL_ERROR_MSG; -use parse::features::suggestions; #[derive(Debug, PartialEq, Copy, Clone)] #[doc(hidden)] @@ -221,8 +220,7 @@ where let mut found = false; let mut foundx2 = false; - for p in positionals!(self.app) - { + for p in positionals!(self.app) { if foundx2 && !p.is_set(ArgSettings::Required) { assert!( p.is_set(ArgSettings::Required), @@ -253,8 +251,7 @@ where // Check that if a required positional argument is found, all positions with a lower // index are also required let mut found = false; - for p in positionals!(self.app) - { + for p in positionals!(self.app) { if found { assert!( p.is_set(ArgSettings::Required), @@ -301,9 +298,8 @@ where // Does all the initializing and prepares the parser pub(crate) fn _build(&mut self) { debugln!("Parser::_build;"); - let mut key: Vec<(KeyType, usize)> = Vec::new(); + let mut key: Vec<(KeyType, usize)> = Vec::new(); for (i, a) in self.app.args.values().enumerate() { - if let Some(ref index) = a.index { key.push((KeyType::Position((*index) as usize), i)); } else { @@ -355,7 +351,13 @@ where .app .args .keys() - .filter(|x| if let KeyType::Position(_) = x { true } else { false }) + .filter(|x| { + if let KeyType::Position(_) = x { + true + } else { + false + } + }) .count()) }) && self.positionals.values().last().map_or(false, |p_name| { !self @@ -529,7 +531,8 @@ where } let low_index_mults = self.is_set(AS::LowIndexMultiplePositional) - && pos_counter == ( + && pos_counter + == ( //TODO make a macro for that self .app @@ -542,9 +545,14 @@ where .app .args .keys() - .filter(|x| if let KeyType::Position(_) = x { true } else { false }) - .count() - 1) - && !self.is_set(AS::TrailingValues)); + .filter(|x| { + if let KeyType::Position(_) = x { + true + } else { + false + } + }) + .count() - 1) && !self.is_set(AS::TrailingValues)); debugln!( "Parser::get_matches_with: Positional counter...{}", pos_counter @@ -586,11 +594,17 @@ where // to the last (highest index) positional debugln!("Parser::get_matches_with: .last(true) and --, setting last pos"); pos_counter = self - .app - .args - .keys() - .filter(|x| if let KeyType::Position(_) = x { true } else { false }) - .count(); + .app + .args + .keys() + .filter(|x| { + if let KeyType::Position(_) = x { + true + } else { + false + } + }) + .count(); } if let Some(p) = positionals!(self.app).find(|p| p.index == Some(pos_counter as u64)) { if p.is_set(ArgSettings::Last) && !self.is_set(AS::TrailingValues) { @@ -602,12 +616,20 @@ where )); } if !self.is_set(AS::TrailingValues) - && (self.is_set(AS::TrailingVarArg) && pos_counter == self - .app - .args - .keys() - .filter(|x| if let KeyType::Position(_) = x { true } else { false }) - .count()) + && (self.is_set(AS::TrailingVarArg) + && pos_counter + == self + .app + .args + .keys() + .filter(|x| { + if let KeyType::Position(_) = x { + true + } else { + false + } + }) + .count()) { self.app.settings.set(AS::TrailingValues); } @@ -1377,7 +1399,6 @@ where $a.name ); $_self.add_val_to_arg($a, OsStr::new(val), $m)?; - } else if $m.get($a.name).is_some() { debugln!( "Parser::add_defaults:iter:{}: has user defined vals", @@ -1470,15 +1491,16 @@ where { fn did_you_mean_error(&self, arg: &str, matcher: &mut ArgMatcher<'a>) -> ClapResult<()> { // Didn't match a flag or option + let longs = longs!(self.app).map(|x| x.to_string_lossy().into_owned()).collect::>(); + let suffix = suggestions::did_you_mean_flag_suffix(arg, longs.iter().map(|ref x| &x[..] ), &*self.app.subcommands); // Add the arg to the matches to build a proper usage string - if let Some(name) = suffix.1 { - if let Some(opt) = self.app.args.get(KeyType::Long(name)) { - for grp in groups_for_arg!(self.app, &opt.name) { - matcher.inc_occurrence_of(&*grp); - } + if let Some(ref name) = suffix.1 { + if let Some(opt) = self.app.args.get(KeyType::Long(OsString::from(name))) { + self.groups_for_arg(&*opt.name) + .and_then(|grps| Some(matcher.inc_occurrences_of(&*grps))); matcher.insert(&*opt.name); } else if let Some(flg) = self.app.args.get(KeyType::Long(&OsStr::new(name))) { self.groups_for_arg(&*flg.name)