From a7a1341cf5f32bf6b5af12af672876a5c9b392bb Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Fri, 23 Mar 2018 16:40:51 -0400 Subject: [PATCH] address suggestions --- src/cargo/core/resolver/conflict_cache.rs | 29 ++++++++++-------- src/cargo/core/resolver/mod.rs | 37 ++++++++++++++--------- tests/testsuite/resolve.rs | 2 +- 3 files changed, 40 insertions(+), 28 deletions(-) diff --git a/src/cargo/core/resolver/conflict_cache.rs b/src/cargo/core/resolver/conflict_cache.rs index 5fcc012c3..91b094acc 100644 --- a/src/cargo/core/resolver/conflict_cache.rs +++ b/src/cargo/core/resolver/conflict_cache.rs @@ -41,13 +41,15 @@ pub(super) struct ConflictCache { } impl ConflictCache { - pub(super) fn new() -> ConflictCache { + pub fn new() -> ConflictCache { ConflictCache { con_from_dep: HashMap::new(), dep_from_pid: HashMap::new(), } } - pub(super) fn filter_conflicting( + /// Finds any known set of conflicts, if any, + /// which are activated in `cx` and pass the `filter` specified? + pub fn find_conflicting( &self, cx: &Context, dep: &Dependency, @@ -56,21 +58,24 @@ impl ConflictCache { where for<'r> F: FnMut(&'r &HashMap) -> bool, { - self.con_from_dep.get(dep).and_then(|past_bad| { - past_bad - .iter() - .filter(filter) - .find(|conflicting| cx.is_conflicting(None, conflicting)) - }) + self.con_from_dep + .get(dep)? + .iter() + .filter(filter) + .find(|conflicting| cx.is_conflicting(None, conflicting)) } - pub(super) fn conflicting( + pub fn conflicting( &self, cx: &Context, dep: &Dependency, ) -> Option<&HashMap> { - self.filter_conflicting(cx, dep, |_| true) + self.find_conflicting(cx, dep, |_| true) } - pub(super) fn insert(&mut self, dep: &Dependency, con: &HashMap) { + + /// Add to the cache a conflict of the form: + /// `dep` is known to be unresolvable if + /// all the `PackageId` entries are activated + pub fn insert(&mut self, dep: &Dependency, con: &HashMap) { let past = self.con_from_dep .entry(dep.clone()) .or_insert_with(Vec::new); @@ -85,7 +90,7 @@ impl ConflictCache { } } } - pub(super) fn get_dep_from_pid(&self, pid: &PackageId) -> Option<&HashSet> { + pub fn dependencies_conflicting_with(&self, pid: &PackageId) -> Option<&HashSet> { self.dep_from_pid.get(pid) } } diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index 41fc8aecb..78a9afea2 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -596,12 +596,12 @@ impl DepsFrame { .unwrap_or(0) } - fn flatten<'s>(&'s self) -> Box + 's> { + fn flatten<'s>(&'s self) -> Box + 's> { // TODO: with impl Trait the Box can be removed Box::new( self.remaining_siblings .clone() - .map(move |(_, (d, _, _))| (self.parent.package_id().clone(), d)), + .map(move |(_, (d, _, _))| (self.parent.package_id(), d)), ) } } @@ -1238,18 +1238,20 @@ fn activate_deps_loop( }) .next() { - let mut conflicting = conflicting.clone(); - - // If one of our deps conflicts with + // If one of our deps is known unresolvable // then we will not succeed. // How ever if we are part of the reason that // one of our deps conflicts then // we can make a stronger statement // because we will definitely be activated when // we try our dep. - conflicting.remove(&pid); + conflicting_activations.extend( + conflicting + .iter() + .filter(|&(p, _)| p != &pid) + .map(|(p, r)| (p.clone(), r.clone())), + ); - conflicting_activations.extend(conflicting); has_past_conflicting_dep = true; } } @@ -1261,16 +1263,18 @@ fn activate_deps_loop( // ourselves are incompatible with that dep, so we know that deps // parent conflict with us. if !has_past_conflicting_dep { - if let Some(rel_deps) = past_conflicting_activations.get_dep_from_pid(&pid) + if let Some(known_related_bad_deps) = + past_conflicting_activations.dependencies_conflicting_with(&pid) { if let Some((other_parent, conflict)) = remaining_deps .iter() .flat_map(|other| other.flatten()) // for deps related to us - .filter(|&(_, ref other_dep)| rel_deps.contains(other_dep)) + .filter(|&(_, ref other_dep)| + known_related_bad_deps.contains(other_dep)) .filter_map(|(other_parent, other_dep)| { past_conflicting_activations - .filter_conflicting( + .find_conflicting( &cx, &other_dep, |con| con.contains_key(&pid) @@ -1279,8 +1283,8 @@ fn activate_deps_loop( }) .next() { - let mut conflict = conflict.clone(); let rel = conflict.get(&pid).unwrap().clone(); + // The conflict we found is // "other dep will not succeed if we are activated." // We want to add @@ -1288,10 +1292,13 @@ fn activate_deps_loop( // but that is not how the cache is set up. // So we add the less general but much faster, // "our dep will not succeed if other dep's parent is activated". - conflict.insert(other_parent.clone(), rel); - conflict.remove(&pid); - - conflicting_activations.extend(conflict); + conflicting_activations.extend( + conflict + .iter() + .filter(|&(p, _)| p != &pid) + .map(|(p, r)| (p.clone(), r.clone())), + ); + conflicting_activations.insert(other_parent.clone(), rel); has_past_conflicting_dep = true; } } diff --git a/tests/testsuite/resolve.rs b/tests/testsuite/resolve.rs index bff6d88a1..8ec3a6e46 100644 --- a/tests/testsuite/resolve.rs +++ b/tests/testsuite/resolve.rs @@ -825,7 +825,7 @@ fn resolving_with_constrained_cousins_backtrack() { reglist.push( pkg!((format!("level{}", DEPTH).as_str(), "1.0.0") => [dep_req("backtrack_trap0", "*"), dep_req("cloaking", "*") - ]), + ]), ); let reg = registry(reglist.clone()); -- 2.30.2