From 3a484480bf31d3d976de0fa1987fe94cf6971c1e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20H=C3=B8rl=C3=BCck=20Berg?=
 <36937807+henrikhorluck@users.noreply.github.com>
Date: Mon, 31 Jul 2023 12:58:17 +0200
Subject: [PATCH] Remove premature optimization

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 https://github.com/rust-lang/rust/commit/db1b1919baba8be48d997d9f70a6a5df7e31612a
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
---
 fish-rust/src/builtins/path.rs | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/fish-rust/src/builtins/path.rs b/fish-rust/src/builtins/path.rs
index 471ea292e..87e410df8 100644
--- a/fish-rust/src/builtins/path.rs
+++ b/fish-rust/src/builtins/path.rs
@@ -1,6 +1,5 @@
 use crate::env::environment::Environment;
 use std::cmp::Ordering;
-use std::collections::HashMap;
 use std::os::unix::prelude::{FileTypeExt, MetadataExt};
 use std::time::SystemTime;
 
@@ -735,19 +734,12 @@ fn path_sort(
         false => SplitBehavior::InferNull,
     });
 
-    let owning_list: Vec<_> = arguments.map(|(f, _)| f).collect();
-    let mut list: Vec<&wstr> = owning_list.iter().map(|f| f.as_ref()).collect();
+    let mut list: Vec<_> = arguments.map(|(f, _)| f).collect();
 
     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
         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
                 Ordering::Equal => Ordering::Greater,
                 order if opts.reverse => order.reverse(),
@@ -757,7 +749,7 @@ fn path_sort(
 
         if opts.unique {
             // 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 {
         // Without --key, we just sort by the entire path,
@@ -778,7 +770,7 @@ fn path_sort(
     }
 
     for entry in list {
-        path_out(streams, &opts, entry);
+        path_out(streams, &opts, &entry);
     }
 
     /* TODO: Return true only if already sorted? */