From b6d96cac040d6ff84b95f5934d73e2d99ad3bbf5 Mon Sep 17 00:00:00 2001 From: Urgau Date: Wed, 25 Sep 2024 11:02:30 +0200 Subject: [PATCH] Change signature of `get_many_mut` APIs by 1) panicking on overlapping keys and 2) returning an array of Option rather than an Option of array. --- src/map.rs | 134 +++++++++++++++++++++++++++++++------------------ src/raw/mod.rs | 34 +++++++------ src/table.rs | 50 ++++++++++++------ 3 files changed, 139 insertions(+), 79 deletions(-) diff --git a/src/map.rs b/src/map.rs index 1e794ca4f..eef6afb6f 100644 --- a/src/map.rs +++ b/src/map.rs @@ -1470,6 +1470,10 @@ where /// mutable reference will be returned to any value. `None` will be returned if any of the /// keys are duplicates or missing. /// + /// # Panics + /// + /// Panics if some keys are overlapping. + /// /// # Examples /// /// ``` @@ -1487,27 +1491,33 @@ where /// ]); /// assert_eq!( /// got, - /// Some([ - /// &mut 1807, - /// &mut 1800, - /// ]), + /// [ + /// Some(&mut 1807), + /// Some(&mut 1800), + /// ], /// ); /// /// // Missing keys result in None /// let got = libraries.get_many_mut([ - /// "Athenæum", + /// "Athenæum1", /// "New York Public Library", /// ]); - /// assert_eq!(got, None); + /// assert_eq!(got, [None, None]); + /// ``` + /// + /// ```should_panic + /// use hashbrown::HashMap; + /// + /// let mut libraries = HashMap::new(); + /// libraries.insert("Athenæum".to_string(), 1807); /// - /// // Duplicate keys result in None + /// // Duplicate keys panic! /// let got = libraries.get_many_mut([ /// "Athenæum", /// "Athenæum", /// ]); - /// assert_eq!(got, None); /// ``` - pub fn get_many_mut(&mut self, ks: [&Q; N]) -> Option<[&'_ mut V; N]> + pub fn get_many_mut(&mut self, ks: [&Q; N]) -> [Option<&'_ mut V>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1517,8 +1527,8 @@ where /// Attempts to get mutable references to `N` values in the map at once, without validating that /// the values are unique. /// - /// Returns an array of length `N` with the results of each query. `None` will be returned if - /// any of the keys are missing. + /// Returns an array of length `N` with the results of each query. `None` will be used if + /// the key is missing. /// /// For a safe alternative see [`get_many_mut`](`HashMap::get_many_mut`). /// @@ -1540,29 +1550,31 @@ where /// libraries.insert("Herzogin-Anna-Amalia-Bibliothek".to_string(), 1691); /// libraries.insert("Library of Congress".to_string(), 1800); /// - /// let got = libraries.get_many_mut([ + /// // SAFETY: The keys do not overlap. + /// let got = unsafe { libraries.get_many_unchecked_mut([ /// "Athenæum", /// "Library of Congress", - /// ]); + /// ]) }; /// assert_eq!( /// got, - /// Some([ - /// &mut 1807, - /// &mut 1800, - /// ]), + /// [ + /// Some(&mut 1807), + /// Some(&mut 1800), + /// ], /// ); /// - /// // Missing keys result in None - /// let got = libraries.get_many_mut([ + /// // SAFETY: The keys do not overlap. + /// let got = unsafe { libraries.get_many_unchecked_mut([ /// "Athenæum", /// "New York Public Library", - /// ]); - /// assert_eq!(got, None); + /// ]) }; + /// // Missing keys result in None + /// assert_eq!(got, [Some(&mut 1807), None]); /// ``` pub unsafe fn get_many_unchecked_mut( &mut self, ks: [&Q; N], - ) -> Option<[&'_ mut V; N]> + ) -> [Option<&'_ mut V>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1574,8 +1586,11 @@ where /// references to the corresponding keys. /// /// Returns an array of length `N` with the results of each query. For soundness, at most one - /// mutable reference will be returned to any value. `None` will be returned if any of the keys - /// are duplicates or missing. + /// mutable reference will be returned to any value. `None` will be used if the key is missing. + /// + /// # Panics + /// + /// Panics if some keys are overlapping. /// /// # Examples /// @@ -1594,30 +1609,37 @@ where /// ]); /// assert_eq!( /// got, - /// Some([ - /// (&"Bodleian Library".to_string(), &mut 1602), - /// (&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691), - /// ]), + /// [ + /// Some((&"Bodleian Library".to_string(), &mut 1602)), + /// Some((&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691)), + /// ], /// ); /// // Missing keys result in None /// let got = libraries.get_many_key_value_mut([ /// "Bodleian Library", /// "Gewandhaus", /// ]); - /// assert_eq!(got, None); + /// assert_eq!(got, [Some((&"Bodleian Library".to_string(), &mut 1602)), None]); + /// ``` + /// + /// ```should_panic + /// use hashbrown::HashMap; + /// + /// let mut libraries = HashMap::new(); + /// libraries.insert("Bodleian Library".to_string(), 1602); + /// libraries.insert("Herzogin-Anna-Amalia-Bibliothek".to_string(), 1691); /// - /// // Duplicate keys result in None + /// // Duplicate keys result in panic! /// let got = libraries.get_many_key_value_mut([ /// "Bodleian Library", /// "Herzogin-Anna-Amalia-Bibliothek", /// "Herzogin-Anna-Amalia-Bibliothek", /// ]); - /// assert_eq!(got, None); /// ``` pub fn get_many_key_value_mut( &mut self, ks: [&Q; N], - ) -> Option<[(&'_ K, &'_ mut V); N]> + ) -> [Option<(&'_ K, &'_ mut V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1657,22 +1679,28 @@ where /// ]); /// assert_eq!( /// got, - /// Some([ - /// (&"Bodleian Library".to_string(), &mut 1602), - /// (&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691), - /// ]), + /// [ + /// Some((&"Bodleian Library".to_string(), &mut 1602)), + /// Some((&"Herzogin-Anna-Amalia-Bibliothek".to_string(), &mut 1691)), + /// ], /// ); /// // Missing keys result in None /// let got = libraries.get_many_key_value_mut([ /// "Bodleian Library", /// "Gewandhaus", /// ]); - /// assert_eq!(got, None); + /// assert_eq!( + /// got, + /// [ + /// Some((&"Bodleian Library".to_string(), &mut 1602)), + /// None, + /// ], + /// ); /// ``` pub unsafe fn get_many_key_value_unchecked_mut( &mut self, ks: [&Q; N], - ) -> Option<[(&'_ K, &'_ mut V); N]> + ) -> [Option<(&'_ K, &'_ mut V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1680,7 +1708,7 @@ where .map(|res| res.map(|(k, v)| (&*k, v))) } - fn get_many_mut_inner(&mut self, ks: [&Q; N]) -> Option<[&'_ mut (K, V); N]> + fn get_many_mut_inner(&mut self, ks: [&Q; N]) -> [Option<&'_ mut (K, V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -1692,7 +1720,7 @@ where unsafe fn get_many_unchecked_mut_inner( &mut self, ks: [&Q; N], - ) -> Option<[&'_ mut (K, V); N]> + ) -> [Option<&'_ mut (K, V)>; N] where Q: Hash + Equivalent + ?Sized, { @@ -5937,7 +5965,7 @@ mod test_map { } #[test] - fn test_get_each_mut() { + fn test_get_many_mut() { let mut map = HashMap::new(); map.insert("foo".to_owned(), 0); map.insert("bar".to_owned(), 10); @@ -5945,25 +5973,31 @@ mod test_map { map.insert("qux".to_owned(), 30); let xs = map.get_many_mut(["foo", "qux"]); - assert_eq!(xs, Some([&mut 0, &mut 30])); + assert_eq!(xs, [Some(&mut 0), Some(&mut 30)]); let xs = map.get_many_mut(["foo", "dud"]); - assert_eq!(xs, None); - - let xs = map.get_many_mut(["foo", "foo"]); - assert_eq!(xs, None); + assert_eq!(xs, [Some(&mut 0), None]); let ys = map.get_many_key_value_mut(["bar", "baz"]); assert_eq!( ys, - Some([(&"bar".to_owned(), &mut 10), (&"baz".to_owned(), &mut 20),]), + [ + Some((&"bar".to_owned(), &mut 10)), + Some((&"baz".to_owned(), &mut 20)) + ], ); let ys = map.get_many_key_value_mut(["bar", "dip"]); - assert_eq!(ys, None); + assert_eq!(ys, [Some((&"bar".to_string(), &mut 10)), None]); + } + + #[test] + #[should_panic = "duplicate keys found"] + fn test_get_many_mut_duplicate() { + let mut map = HashMap::new(); + map.insert("foo".to_owned(), 0); - let ys = map.get_many_key_value_mut(["baz", "baz"]); - assert_eq!(ys, None); + let _xs = map.get_many_mut(["foo", "foo"]); } #[test] diff --git a/src/raw/mod.rs b/src/raw/mod.rs index 2c6392181..887d96152 100644 --- a/src/raw/mod.rs +++ b/src/raw/mod.rs @@ -1291,20 +1291,25 @@ impl RawTable { &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { + ) -> [Option<&'_ mut T>; N] { unsafe { - let ptrs = self.get_many_mut_pointers(hashes, eq)?; + let ptrs = self.get_many_mut_pointers(hashes, eq); for (i, &cur) in ptrs.iter().enumerate() { - if ptrs[..i].iter().any(|&prev| ptr::eq::(prev, cur)) { - return None; + if let Some(cur) = cur { + if ptrs[..i] + .iter() + .flatten() + .any(|&prev| ptr::eq::(prev, cur)) + { + panic!("duplicate keys found"); + } } } // All bucket are distinct from all previous buckets so we're clear to return the result // of the lookup. - // TODO use `MaybeUninit::array_assume_init` here instead once that's stable. - Some(mem::transmute_copy(&ptrs)) + ptrs.map(|ptr| ptr.map(|ptr| &mut *ptr)) } } @@ -1312,27 +1317,26 @@ impl RawTable { &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { - let ptrs = self.get_many_mut_pointers(hashes, eq)?; - Some(mem::transmute_copy(&ptrs)) + ) -> [Option<&'_ mut T>; N] { + let ptrs = self.get_many_mut_pointers(hashes, eq); + ptrs.map(|ptr| ptr.map(|ptr| &mut *ptr)) } unsafe fn get_many_mut_pointers( &mut self, hashes: [u64; N], mut eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[*mut T; N]> { + ) -> [Option<*mut T>; N] { // TODO use `MaybeUninit::uninit_array` here instead once that's stable. - let mut outs: MaybeUninit<[*mut T; N]> = MaybeUninit::uninit(); + let mut outs: MaybeUninit<[Option<*mut T>; N]> = MaybeUninit::uninit(); let outs_ptr = outs.as_mut_ptr(); for (i, &hash) in hashes.iter().enumerate() { - let cur = self.find(hash, |k| eq(i, k))?; - *(*outs_ptr).get_unchecked_mut(i) = cur.as_mut(); + let cur = self.find(hash, |k| eq(i, k)).map(|cur| cur.as_ptr()); + *(*outs_ptr).get_unchecked_mut(i) = cur; } - // TODO use `MaybeUninit::array_assume_init` here instead once that's stable. - Some(outs.assume_init()) + outs.assume_init() } /// Returns the number of elements the map can hold without reallocating. diff --git a/src/table.rs b/src/table.rs index 8f9530404..960d5d1d8 100644 --- a/src/table.rs +++ b/src/table.rs @@ -969,6 +969,10 @@ where /// mutable reference will be returned to any value. `None` will be returned if any of the /// keys are duplicates or missing. /// + /// # Panics + /// + /// Panics if some keys are overlapping. + /// /// # Examples /// /// ``` @@ -994,29 +998,52 @@ where /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); /// assert_eq!( /// got, - /// Some([&mut ("Athenæum", 1807), &mut ("Library of Congress", 1800),]), + /// [Some(&mut ("Athenæum", 1807)), Some(&mut ("Library of Congress", 1800))], /// ); /// /// // Missing keys result in None /// let keys = ["Athenæum", "New York Public Library"]; /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); + /// assert_eq!(got, [Some(&mut ("Athenæum", 1807)), None]); + /// # } + /// # fn main() { + /// # #[cfg(feature = "nightly")] + /// # test() + /// # } + /// ``` + /// + /// ```should_panic + /// # #[cfg(feature = "nightly")] + /// # fn test() { + /// # use hashbrown::{HashTable, DefaultHashBuilder}; + /// # use std::hash::BuildHasher; + /// + /// let mut libraries: HashTable<(&str, u32)> = HashTable::new(); + /// let hasher = DefaultHashBuilder::default(); + /// let hasher = |val: &_| hasher.hash_one(val); + /// for (k, v) in [ + /// ("Athenæum", 1807), + /// ("Library of Congress", 1800), + /// ] { + /// libraries.insert_unique(hasher(&k), (k, v), |(k, _)| hasher(&k)); + /// } /// - /// // Duplicate keys result in None + /// // Duplicate keys result in a panic! /// let keys = ["Athenæum", "Athenæum"]; /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); /// # } /// # fn main() { /// # #[cfg(feature = "nightly")] - /// # test() + /// # test(); + /// # #[cfg(not(feature = "nightly"))] + /// # panic!(); /// # } /// ``` pub fn get_many_mut( &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { + ) -> [Option<&'_ mut T>; N] { self.raw.get_many_mut(hashes, eq) } @@ -1063,18 +1090,13 @@ where /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); /// assert_eq!( /// got, - /// Some([&mut ("Athenæum", 1807), &mut ("Library of Congress", 1800),]), + /// [Some(&mut ("Athenæum", 1807)), Some(&mut ("Library of Congress", 1800))], /// ); /// /// // Missing keys result in None /// let keys = ["Athenæum", "New York Public Library"]; /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); - /// - /// // Duplicate keys result in None - /// let keys = ["Athenæum", "Athenæum"]; - /// let got = libraries.get_many_mut(keys.map(|k| hasher(&k)), |i, val| keys[i] == val.0); - /// assert_eq!(got, None); + /// assert_eq!(got, [Some(&mut ("Athenæum", 1807)), None]); /// # } /// # fn main() { /// # #[cfg(feature = "nightly")] @@ -1085,7 +1107,7 @@ where &mut self, hashes: [u64; N], eq: impl FnMut(usize, &T) -> bool, - ) -> Option<[&'_ mut T; N]> { + ) -> [Option<&'_ mut T>; N] { self.raw.get_many_unchecked_mut(hashes, eq) }