From 6a71d32cec4bbf3533ddd09a8aba3853d5fa2af8 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Wed, 21 Feb 2024 21:00:38 -0500 Subject: [PATCH] use impl IntoIterator to avoide clones (#24) Co-authored-by: Jacob Finkelman --- src/internal/core.rs | 6 +++--- src/internal/incompatibility.rs | 11 +++++++---- src/solver.rs | 28 ++++++++++++++++------------ src/type_aliases.rs | 10 +--------- tests/proptest.rs | 4 ++-- tests/sat_dependency_provider.rs | 4 ++-- 6 files changed, 31 insertions(+), 32 deletions(-) diff --git a/src/internal/core.rs b/src/internal/core.rs index 6c3b5d55..84843b97 100644 --- a/src/internal/core.rs +++ b/src/internal/core.rs @@ -16,7 +16,7 @@ use crate::internal::partial_solution::{DecisionLevel, PartialSolution}; use crate::internal::small_vec::SmallVec; use crate::report::DerivationTree; use crate::solver::DependencyProvider; -use crate::type_aliases::{DependencyConstraints, IncompDpId, Map}; +use crate::type_aliases::{IncompDpId, Map}; use crate::version_set::VersionSet; /// Current state of the PubGrub algorithm. @@ -85,12 +85,12 @@ impl State { &mut self, package: DP::P, version: DP::V, - deps: &DependencyConstraints, + deps: impl IntoIterator, ) -> std::ops::Range> { // Create incompatibilities and allocate them in the store. let new_incompats_id_range = self.incompatibility_store - .alloc_iter(deps.iter().map(|dep| { + .alloc_iter(deps.into_iter().map(|dep| { Incompatibility::from_dependency( package.clone(), ::singleton(version.clone()), diff --git a/src/internal/incompatibility.rs b/src/internal/incompatibility.rs index 1e4d2346..2cf8bf09 100644 --- a/src/internal/incompatibility.rs +++ b/src/internal/incompatibility.rs @@ -139,10 +139,10 @@ impl Incompatibilit } /// Build an incompatibility from a given dependency. - pub fn from_dependency(package: P, versions: VS, dep: (&P, &VS)) -> Self { + pub fn from_dependency(package: P, versions: VS, dep: (P, VS)) -> Self { let (p2, set2) = dep; Self { - package_terms: if set2 == &VS::empty() { + package_terms: if set2 == VS::empty() { SmallMap::One([(package.clone(), Term::Positive(versions.clone()))]) } else { SmallMap::Two([ @@ -150,7 +150,7 @@ impl Incompatibilit (p2.clone(), Term::Negative(set2.clone())), ]) }, - kind: Kind::FromDependencyOf(package, versions, p2.clone(), set2.clone()), + kind: Kind::FromDependencyOf(package, versions, p2, set2), } } @@ -193,7 +193,10 @@ impl Incompatibilit .unwrap() .unwrap_positive() .union(other.get(p1).unwrap().unwrap_positive()), // It is safe to `simplify` here - (p2, dep_term.map_or(&VS::empty(), |v| v.unwrap_negative())), + ( + p2.clone(), + dep_term.map_or(VS::empty(), |v| v.unwrap_negative().clone()), + ), )); } diff --git a/src/solver.rs b/src/solver.rs index a18bacc9..25505740 100644 --- a/src/solver.rs +++ b/src/solver.rs @@ -71,7 +71,7 @@ use crate::error::PubGrubError; pub use crate::internal::core::State; pub use crate::internal::incompatibility::{Incompatibility, Kind}; use crate::package::Package; -use crate::type_aliases::{DependencyConstraints, Map, SelectedDependencies}; +use crate::type_aliases::{Map, SelectedDependencies}; use crate::version_set::VersionSet; use log::{debug, info}; @@ -159,10 +159,10 @@ pub fn resolve( )); continue; } - Dependencies::Available(x) if x.contains_key(p) => { + Dependencies::Available(x) if x.clone().into_iter().any(|(d, _)| &d == p) => { return Err(PubGrubError::SelfDependency { package: p.clone(), - version: v, + version: v.clone(), }); } Dependencies::Available(x) => x, @@ -172,12 +172,12 @@ pub fn resolve( let dep_incompats = state.add_incompatibility_from_dependencies( p.clone(), v.clone(), - &known_dependencies, + known_dependencies, ); state.partial_solution.add_version( p.clone(), - v, + v.clone(), dep_incompats, &state.incompatibility_store, ); @@ -197,7 +197,7 @@ pub enum Dependencies), + Available(T), } /// Trait that allows the algorithm to retrieve available packages and their dependencies. @@ -284,7 +284,7 @@ pub trait DependencyProvider { &self, package: &Self::P, version: &Self::V, - ) -> Result, Self::Err>; + ) -> Result + Clone, Self::M>, Self::Err>; /// This is called fairly regularly during the resolution, /// if it returns an Err then resolution will be terminated. @@ -308,7 +308,7 @@ pub trait DependencyProvider { )] #[cfg_attr(feature = "serde", serde(transparent))] pub struct OfflineDependencyProvider { - dependencies: Map>>, + dependencies: Map>>, } impl OfflineDependencyProvider { @@ -358,8 +358,8 @@ impl OfflineDependencyProvider { /// Lists dependencies of a given package and version. /// Returns [None] if no information is available regarding that package and version pair. - fn dependencies(&self, package: &P, version: &VS::V) -> Option> { - self.dependencies.get(package)?.get(version).cloned() + fn dependencies(&self, package: &P, version: &VS::V) -> Option<&Map> { + self.dependencies.get(package)?.get(version) } } @@ -397,12 +397,16 @@ impl DependencyProvider for OfflineDependencyProvide &self, package: &P, version: &VS::V, - ) -> Result, Infallible> { + ) -> Result + Clone, Self::M>, Infallible> { Ok(match self.dependencies(package, version) { None => { Dependencies::Unavailable("its dependencies could not be determined".to_string()) } - Some(dependencies) => Dependencies::Available(dependencies), + Some(dependencies) => Dependencies::Available( + dependencies + .into_iter() + .map(|(p, vs)| (p.clone(), vs.clone())), + ), }) } } diff --git a/src/type_aliases.rs b/src/type_aliases.rs index 8b893d18..25850ac1 100644 --- a/src/type_aliases.rs +++ b/src/type_aliases.rs @@ -10,18 +10,10 @@ pub type Map = rustc_hash::FxHashMap; /// Set implementation used by the library. pub type Set = rustc_hash::FxHashSet; -/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve) -/// from [DependencyConstraints]. +/// Concrete dependencies picked by the library during [resolve](crate::solver::resolve). pub type SelectedDependencies = Map<::P, ::V>; -/// Holds information about all possible versions a given package can accept. -/// There is a difference in semantics between an empty map -/// inside [DependencyConstraints] and [Dependencies::Unavailable](crate::solver::Dependencies::Unavailable): -/// the former means the package has no dependency and it is a known fact, -/// while the latter means they could not be fetched by the [DependencyProvider]. -pub type DependencyConstraints = Map; - pub(crate) type IncompDpId = IncompId< ::P, ::VS, diff --git a/tests/proptest.rs b/tests/proptest.rs index 33e5cece..d3284756 100644 --- a/tests/proptest.rs +++ b/tests/proptest.rs @@ -352,8 +352,8 @@ fn retain_dependencies( smaller_dependency_provider.add_dependencies( n.clone(), v.clone(), - deps.iter().filter_map(|(dep, range)| { - if !retain(n, v, dep) { + deps.into_iter().filter_map(|(dep, range)| { + if !retain(n, v, &dep) { None } else { Some((dep.clone(), range.clone())) diff --git a/tests/sat_dependency_provider.rs b/tests/sat_dependency_provider.rs index ec496c1b..0f44b5e8 100644 --- a/tests/sat_dependency_provider.rs +++ b/tests/sat_dependency_provider.rs @@ -69,10 +69,10 @@ impl SatResolve { Dependencies::Unavailable(_) => panic!(), Dependencies::Available(d) => d, }; - for (p1, range) in &deps { + for (p1, range) in deps { let empty_vec = vec![]; let mut matches: Vec = all_versions_by_p - .get(p1) + .get(&p1) .unwrap_or(&empty_vec) .iter() .filter(|(v1, _)| range.contains(v1))