Skip to content

Commit

Permalink
What if we have sorted?
Browse files Browse the repository at this point in the history
Note that all non-changed `.sort` calls are marked with `// no
change`.

There 15 changed and 5 non changed occurrences of .sort
  • Loading branch information
matklad committed May 5, 2018
1 parent 5986492 commit 3ff7ccc
Show file tree
Hide file tree
Showing 17 changed files with 43 additions and 47 deletions.
8 changes: 4 additions & 4 deletions src/bin/cargo/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,12 @@ fn find_closest(config: &Config, cmd: &str) -> Option<String> {
let cmds = list_commands(config);
// Only consider candidates with a lev_distance of 3 or less so we don't
// suggest out-of-the-blue options.
let mut filtered = cmds.iter()
cmds.iter()
.map(|&(ref c, _)| (lev_distance(c, cmd), c))
.filter(|&(d, _)| d < 4)
.collect::<Vec<_>>();
filtered.sort_by(|a, b| a.0.cmp(&b.0));
filtered.get(0).map(|slot| slot.1.clone())
.collect::<Vec<_>>()
.sorted_by(|a, b| a.0.cmp(&b.0))
.get(0).map(|slot| slot.1.clone())

This comment has been minimized.

Copy link
@RalfJung

RalfJung Mar 15, 2020

If you only ever get the first element, wouldn't it be much more efficient to search for minimum/maximum than to sort the entire vector?

This comment has been minimized.

Copy link
@matklad

matklad Mar 15, 2020

Author Owner

Makes sense! I haven’t actually read this code, just mechanically fixed it :)

}

fn execute_external_subcommand(config: &Config, cmd: &str, args: &[&str]) -> CliResult {
Expand Down
3 changes: 1 addition & 2 deletions src/cargo/core/compiler/build_context/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,7 @@ fn env_args(
// ordering through sorting for now. We may perhaps one day wish to
// ensure a deterministic ordering via the order keys were defined
// in files perhaps.
let mut cfgs = cfgs.collect::<Vec<_>>();
cfgs.sort();
let cfgs = cfgs.collect::<Vec<_>>().sorted();

for n in cfgs {
let key = format!("target.{}.{}", n, name);
Expand Down
14 changes: 6 additions & 8 deletions src/cargo/core/compiler/context/compilation_files.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,14 +414,12 @@ fn compute_metadata<'a, 'cfg>(
.hash(&mut hasher);

// Mix in the target-metadata of all the dependencies of this target
{
let mut deps_metadata = cx.dep_targets(unit)
.iter()
.map(|dep| metadata_of(dep, cx, metas))
.collect::<Vec<_>>();
deps_metadata.sort();
deps_metadata.hash(&mut hasher);
}
cx.dep_targets(unit)
.iter()
.map(|dep| metadata_of(dep, cx, metas))
.collect::<Vec<_>>()
.sorted()
.hash(&mut hasher);

// Throw in the profile we're compiling with. This helps caching
// panic=abort and panic=unwind artifacts, additionally with various
Expand Down
3 changes: 1 addition & 2 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,7 @@ pub fn build_map<'b, 'cfg>(cx: &mut Context<'b, 'cfg>, units: &[Unit<'b>]) -> Ca
// to rustc invocation caching schemes, so be sure to generate the same
// set of build script dependency orderings via sorting the targets that
// come out of the `Context`.
let mut targets = cx.dep_targets(unit);
targets.sort_by_key(|u| u.pkg.package_id());
let targets = cx.dep_targets(unit).sorted_by_key(|u| u.pkg.package_id());

for unit in targets.iter() {
let dep_scripts = build(out, cx, unit)?;
Expand Down
3 changes: 1 addition & 2 deletions src/cargo/core/compiler/fingerprint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,7 @@ fn calculate<'a, 'cfg>(
let fingerprint = pkg_fingerprint(&cx.bcx, unit.pkg)?;
LocalFingerprint::Precalculated(fingerprint)
};
let mut deps = deps;
deps.sort_by(|&(ref a, _, _), &(ref b, _, _)| a.cmp(b));
let deps = deps.sorted_by(|&(ref a, _, _), &(ref b, _, _)| a.cmp(b));
let extra_flags = if unit.mode.is_doc() {
bcx.rustdocflags_args(unit)?
} else {
Expand Down
1 change: 1 addition & 0 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ impl TargetConfig {
for (k, value) in value.table(&lib_name)?.0 {
pairs.push((k, value));
}
// no change
pairs.sort_by_key(|p| p.0);
for (k, value) in pairs {
let key = format!("{}.{}", key, k);
Expand Down
16 changes: 7 additions & 9 deletions src/cargo/core/resolver/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,19 +111,17 @@ impl Context {

// Next, transform all dependencies into a list of possible candidates
// which can satisfy that dependency.
let mut deps = deps.into_iter()
let deps = deps.into_iter()
.map(|(dep, features)| {
let candidates = registry.query(&dep)?;
Ok((dep, candidates, Rc::new(features)))
})
.collect::<CargoResult<Vec<DepInfo>>>()?;

// Attempt to resolve dependencies with fewer candidates before trying
// dependencies with more candidates. This way if the dependency with
// only one candidate can't be resolved we don't have to do a bunch of
// work before we figure that out.
deps.sort_by_key(|&(_, ref a, _)| a.len());

.collect::<CargoResult<Vec<DepInfo>>>()?
// Attempt to resolve dependencies with fewer candidates before trying
// dependencies with more candidates. This way if the dependency with
// only one candidate can't be resolved we don't have to do a bunch of
// work before we figure that out.
.sorted_by_key(|&(_, ref a, _)| a.len());
Ok(deps)
}

Expand Down
9 changes: 4 additions & 5 deletions src/cargo/core/resolver/encode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -346,8 +346,7 @@ impl<'a, 'cfg> ser::Serialize for WorkspaceResolve<'a, 'cfg> {
where
S: ser::Serializer,
{
let mut ids: Vec<_> = self.resolve.iter().collect();
ids.sort();
let ids: Vec<_> = self.resolve.iter().collect().sorted();

let encodable = ids.iter()
.filter_map(|&id| Some(encodable_resolve_node(id, self.resolve)))
Expand Down Expand Up @@ -396,11 +395,11 @@ fn encodable_resolve_node(id: &PackageId, resolve: &Resolve) -> EncodableDepende
let (replace, deps) = match resolve.replacement(id) {
Some(id) => (Some(encodable_package_id(id)), None),
None => {
let mut deps = resolve
let deps = resolve
.deps_not_replaced(id)
.map(encodable_package_id)
.collect::<Vec<_>>();
deps.sort();
.collect::<Vec<_>>()
.sorted();
(None, Some(deps))
}
};
Expand Down
10 changes: 5 additions & 5 deletions src/cargo/core/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -883,9 +883,10 @@ fn activation_error(
.collect::<Vec<_>>()
.join(", "));

let mut conflicting_activations: Vec<_> = conflicting_activations.iter().collect();
conflicting_activations.sort_unstable();
let (links_errors, mut other_errors): (Vec<_>, Vec<_>) = conflicting_activations
.iter()
.collect::<Vec<_>>()
.sorted_unstable()
.drain(..)
.rev()
.partition(|&(_, r)| r.is_links());
Expand Down Expand Up @@ -951,11 +952,10 @@ fn activation_error(
let all_req = semver::VersionReq::parse("*").unwrap();
let mut new_dep = dep.clone();
new_dep.set_version_req(all_req);
let mut candidates = match registry.query_vec(&new_dep) {
Ok(candidates) => candidates,
let candidates = match registry.query_vec(&new_dep) {
Ok(candidates) => candidates.sorted_unstable_by(|a, b| b.version().cmp(a.version())),
Err(e) => return e,
};
candidates.sort_unstable_by(|a, b| b.version().cmp(a.version()));

let mut msg = if !candidates.is_empty() {
let versions = {
Expand Down
5 changes: 2 additions & 3 deletions src/cargo/core/resolver/resolve.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,8 @@ unable to verify that `{0}` is the same as when the lockfile was generated
}

pub fn features_sorted(&self, pkg: &PackageId) -> Vec<&str> {
let mut v = Vec::from_iter(self.features(pkg).iter().map(|s| s.as_ref()));
v.sort();
v
Vec::from_iter(self.features(pkg).iter().map(|s| s.as_ref()))
.sorted()
}

pub fn query(&self, spec: &str) -> CargoResult<&PackageId> {
Expand Down
1 change: 1 addition & 0 deletions src/cargo/core/resolver/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ impl<'a> RegistryQueryer<'a> {
// sorted fashion to pick the "best candidates" first. Currently we try
// prioritized summaries (those in `try_to_use`) and failing that we
// list everything from the maximum version to the lowest version.
// no change
ret.sort_unstable_by(|a, b| {
let a_in_previous = self.try_to_use.contains(a.summary.package_id());
let b_in_previous = self.try_to_use.contains(b.summary.package_id());
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_generate_lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ pub fn update_lockfile(ws: &Workspace, opts: &UpdateOptions) -> CargoResult<()>

for v in changes.values_mut() {
let (ref mut old, ref mut new) = *v;
// no change
old.sort();
new.sort();
let removed = vec_subtract(old, new);
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -518,14 +518,14 @@ where
};
return Ok((pkg.clone(), Box::new(source)));

fn multi_err(kind: &str, mut pkgs: Vec<&Package>) -> String {
pkgs.sort_by(|a, b| a.name().cmp(&b.name()));
fn multi_err(kind: &str, pkgs: Vec<&Package>) -> String {
format!(
"multiple packages with {} found: {}",
kind,
pkgs.iter()
.map(|p| p.name().as_str())
.collect::<Vec<_>>()
.sorted()
.join(", ")
)
}
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ pub fn package(ws: &Workspace, opts: &PackageOpts) -> CargoResult<Option<FileLoc
if include_lockfile(pkg) {
list.push("Cargo.lock".into());
}
// no change
list.sort();
for file in list.iter() {
println!("{}", file.display());
Expand Down
1 change: 1 addition & 0 deletions src/cargo/ops/cargo_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ fn compile_tests<'a>(
let mut compilation = ops::compile(ws, &options.compile_opts)?;
compilation
.tests
// no change
.sort_by(|a, b| (a.0.package_id(), &a.1, &a.2).cmp(&(b.0.package_id(), &b.1, &b.2)));
Ok(compilation)
}
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/sources/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,8 +447,8 @@ impl<'cfg> PathSource<'cfg> {
// TODO: Drop collect and sort after transition period and dropping warning tests.
// See <https://github.com/rust-lang/cargo/issues/4268>
// and <https://github.com/rust-lang/cargo/pull/4270>
let mut entries: Vec<fs::DirEntry> = fs::read_dir(path)?.map(|e| e.unwrap()).collect();
entries.sort_by(|a, b| a.path().as_os_str().cmp(b.path().as_os_str()));
let entries: Vec<fs::DirEntry> = fs::read_dir(path)?.map(|e| e.unwrap()).collect()
.sorted_by(|a, b| a.path().as_os_str().cmp(b.path().as_os_str()));
for entry in entries {
let path = entry.path();
let name = path.file_name().and_then(|s| s.to_str());
Expand Down
6 changes: 3 additions & 3 deletions src/cargo/util/rustc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -238,8 +238,8 @@ fn rustc_fingerprint(path: &Path, rustup_rustc: &Path) -> CargoResult<u64> {
fn process_fingerprint(cmd: &ProcessBuilder) -> u64 {
let mut hasher = SipHasher::new_with_keys(0, 0);
cmd.get_args().hash(&mut hasher);
let mut env = cmd.get_envs().iter().collect::<Vec<_>>();
env.sort();
env.hash(&mut hasher);
cmd.get_envs().iter().collect::<Vec<_>>()
.sorted()
.hash(&mut hasher);
hasher.finish()
}

0 comments on commit 3ff7ccc

Please sign in to comment.