From 49eaa138b6f7b325bcb3b889cf7d0f89fb6069f8 Mon Sep 17 00:00:00 2001 From: Eh2406 Date: Wed, 15 May 2019 12:07:05 -0400 Subject: [PATCH] add comments --- src/cargo/core/resolver/context.rs | 19 +++++++ src/cargo/core/resolver/mod.rs | 80 +++++++++++++++--------------- 2 files changed, 58 insertions(+), 41 deletions(-) diff --git a/src/cargo/core/resolver/context.rs b/src/cargo/core/resolver/context.rs index 7071d88d1aa..c24f2a3515c 100644 --- a/src/cargo/core/resolver/context.rs +++ b/src/cargo/core/resolver/context.rs @@ -161,6 +161,25 @@ impl Context { .and_then(|(s, l)| if s.package_id() == id { Some(*l) } else { None }) } + /// This calculates `is_active` after backtracking to `age` and activating `alternative`. + /// If the package is active returns the `ContextAge` when it was added + /// but only if it is older then `age`. + /// If it is not active but it is the `alternative` then it returns `age`. + pub fn is_older_active_or( + &self, + id: PackageId, + age: ContextAge, + alternative: PackageId, + ) -> Option { + if id == alternative { + // we are imagining that we used other instead + Some(age) + } else { + // we only care about things that are older then critical_age + self.is_active(id).filter(|&other_age| other_age < age) + } + } + /// Checks whether all of `parent` and the keys of `conflicting activations` /// are still active. /// If so returns the `ContextAge` when the newest one was added. diff --git a/src/cargo/core/resolver/mod.rs b/src/cargo/core/resolver/mod.rs index f013a7f73be..2c0eecca99e 100644 --- a/src/cargo/core/resolver/mod.rs +++ b/src/cargo/core/resolver/mod.rs @@ -890,31 +890,38 @@ fn generalize_conflicting( return None; } - let our_activation_keys: HashSet<_> = our_candidates - .iter() - .map(|c| c.package_id().as_activations_key()) - .collect(); + let our_activation_key = { + // TODO: no reason to allocate a `HashSet` we are just going to throw it out if len > 1 + let our_activation_keys: HashSet<_> = our_candidates + .iter() + .map(|c| c.package_id().as_activations_key()) + .collect(); - // if our dep only matches one semver range then we can fast path any other dep - // that also targets that semver range and has no overlap - let our_activation_key = if our_activation_keys.len() == 1 { - our_activation_keys.iter().next().cloned() - } else { - None + // If our dep only matches one semver range then we can fast path any other dep + // that also targets that semver range and has no overlap. + if our_activation_keys.len() == 1 { + our_activation_keys.iter().next().cloned() + } else { + None + } }; - let our_links: Option> = our_candidates.iter().map(|c| c.links()).collect(); + let our_link = { + // TODO: no reason to allocate a `HashSet` we are just going to throw it out if len > 1 + let our_links: HashSet> = our_candidates.iter().map(|c| c.links()).collect(); - // if our dep only matches things that have a links set then we can fast path any other dep - // that also all use that links and has no overlap - let our_link = if let Some(our_links) = our_links { + // If our dep only matches things that have a links set then we can fast path any other dep + // that also all use that links and has no overlap. if our_links.len() == 1 { - our_links.iter().next().cloned() + // All of `our_candidates` use the same `links`. + // If they all use Some(value), then we can use the fast path. + // If they all use None, then we cant. + our_links.into_iter().next().unwrap() } else { + // Some of `our_candidates` use a different `links` so whatever `links` get used by + // the conflicting dep we can select the other. We cant use the fast path. None } - } else { - None }; let our_candidates: HashSet = @@ -942,11 +949,17 @@ fn generalize_conflicting( .rev() // the last one to be tried is the least likely to be in the cache, so start with that. { - if (our_activation_key - .map_or(false, |our| other.package_id().as_activations_key() == our) - || our_link.map_or(false, |_| other.links() == our_link)) - && !our_candidates.contains(&other.package_id()) + if ( + our_activation_key.map_or(false, |our| { + other.package_id().as_activations_key() == our + }) + // `other` is semver compatible with all of `our_candidates` + || our_link.map_or(false, |_| other.links() == our_link) + // or `other` have the same `links` as all of `our_candidates` + ) && !our_candidates.contains(&other.package_id()) + // and is not one of `our_candidates`. { + // Thus `dep` can not be resolved when `critical_parents_dep` has bean resolved. continue; } @@ -967,16 +980,7 @@ fn generalize_conflicting( if let Some(conflicting) = past_conflicting_activations.find( dep, - &|id| { - if id == other.package_id() { - // we are imagining that we used other instead - Some(backtrack_critical_age) - } else { - cx.is_active(id).filter(|&age| - // we only care about things that are older then critical_age - age < backtrack_critical_age) - } - }, + &|id| cx.is_older_active_or(id, backtrack_critical_age, other.package_id()), Some(other.package_id()), ) { con.extend( @@ -988,6 +992,9 @@ fn generalize_conflicting( continue; } + // We don't have a reason why `other` cant work. + // There for `critical_parents_dep` could have used it, and we can still be activated. + // We can not generalize over `critical_parents_dep`, but maybe the next `critical_parents_dep` continue 'dep; } @@ -1043,16 +1050,7 @@ fn generalize_children_conflicting( .filter_map(|(ref new_dep, _, _)| { past_conflicting_activations.find( new_dep, - &|id| { - if id == candidate.package_id() { - // we are imagining that we used other instead - Some(critical_age) - } else { - cx.is_active(id).filter(|&age| - // we only care about things that are older then critical_age - age < critical_age) - } - }, + &|id| cx.is_older_active_or(id, critical_age, candidate.package_id()), None, ) })