Mirrors/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell synced 2024-12-29 06:13:20 +00:00
Code Issues Projects Releases Packages Wiki Activity

Remove premature optimization

Browse source 
The `impl<T> Hash for &T` hashes the string itself[^1].
It is unclear if that is actually faster than just calling `keyfunc` multiple times (they should all be linear).
For context, Rust by default uses SipHash 1-3 db1b1919ba
An alternative would be to store it as raw pointers aka `*const T`, which have a cheaper hash impl.
That has a more complicated implementation + removes lifetimes.

This commit rather removes the premature optimization.

[^1]: Source: https://doc.rust-lang.org/std/ptr/fn.hash.html
This commit is contained in:
Henrik Hørlück Berg 2023-07-31 12:58:17 +02:00 committed by Peter Ammon
parent 4a4171c34a
commit 3a484480bf
1 changed files with 4 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
fish-rust/src/builtins/path.rs
View file

@ -1,6 +1,5 @@
use crate::env::environment::Environment; use crate::env::environment::Environment;
use std::cmp::Ordering; use std::cmp::Ordering;
use std::collections::HashMap;
use std::os::unix::prelude::{FileTypeExt, MetadataExt}; use std::os::unix::prelude::{FileTypeExt, MetadataExt};
use std::time::SystemTime; use std::time::SystemTime;
@ -735,19 +734,12 @@ fn path_sort(
false => SplitBehavior::InferNull, false => SplitBehavior::InferNull,
}); });
let owning_list: Vec<_> = arguments.map(|(f, _)| f).collect(); let mut list: Vec<_> = arguments.map(|(f, _)| f).collect();
let mut list: Vec<&wstr> = owning_list.iter().map(|f| f.as_ref()).collect();
if opts.key.is_some() { if opts.key.is_some() {
// Keep a map to avoid repeated keyfunc calls
let key: HashMap<&wstr, &wstr> = list
.iter()
.map(|f| (<&wstr>::clone(f), keyfunc(f)))
.collect();
// We use a stable sort here // We use a stable sort here
list.sort_by(|a, b| { list.sort_by(|a, b| {
match wcsfilecmp_glob(key[a], key[b]) { match wcsfilecmp_glob(keyfunc(a), keyfunc(b)) {
// to avoid changing the order so we can chain calls // to avoid changing the order so we can chain calls
Ordering::Equal => Ordering::Greater, Ordering::Equal => Ordering::Greater,
order if opts.reverse => order.reverse(), order if opts.reverse => order.reverse(),
@ -757,7 +749,7 @@ fn path_sort(
if opts.unique { if opts.unique {
// we are sorted, dedup will remove all duplicates // we are sorted, dedup will remove all duplicates
list.dedup_by(|a, b| key[a] == key[b]); list.dedup_by(|a, b| keyfunc(a) == keyfunc(b));
} }
} else { } else {
// Without --key, we just sort by the entire path, // Without --key, we just sort by the entire path,
@ -778,7 +770,7 @@ fn path_sort(
} }
for entry in list { for entry in list {
path_out(streams, &opts, entry); path_out(streams, &opts, &entry);
} }
/* TODO: Return true only if already sorted? */ /* TODO: Return true only if already sorted? */