From 8bceb976df4908b776c7b7e5afddf90b02878590 Mon Sep 17 00:00:00 2001 From: Josh Stone Date: Thu, 12 Sep 2024 08:31:53 -0700 Subject: [PATCH] Wean `HashSet` from the raw-entry API This changes `get_or_insert`, `get_or_insert_with`, and `bitxor_assign` to poke directly at the `RawTable` instead of using `raw_entry_mut()`. `HashSet::get_or_insert_with` also asserts that the converted value is actually equivalent after conversion, so we can ensure set consistency. `HashSet::get_or_insert_owned` is removed for now, since it offers no value over the `_with` method, as we would need to assert that too. --- ci/run.sh | 4 +-- src/set.rs | 102 ++++++++++++++++++++++++----------------------------- 2 files changed, 48 insertions(+), 58 deletions(-) diff --git a/ci/run.sh b/ci/run.sh index fc8755c8f..39cf49c42 100644 --- a/ci/run.sh +++ b/ci/run.sh @@ -43,8 +43,8 @@ if [ "${CHANNEL}" = "nightly" ]; then fi # Make sure we can compile without the default hasher -"${CARGO}" -vv build --target="${TARGET}" --no-default-features --features raw-entry -"${CARGO}" -vv build --target="${TARGET}" --release --no-default-features --features raw-entry +"${CARGO}" -vv build --target="${TARGET}" --no-default-features +"${CARGO}" -vv build --target="${TARGET}" --release --no-default-features "${CARGO}" -vv ${OP} --target="${TARGET}" "${CARGO}" -vv ${OP} --target="${TARGET}" --features "${FEATURES}" diff --git a/src/set.rs b/src/set.rs index 9b818ed69..a93c0928f 100644 --- a/src/set.rs +++ b/src/set.rs @@ -1,5 +1,4 @@ use crate::{Equivalent, TryReserveError}; -use alloc::borrow::ToOwned; use core::hash::{BuildHasher, Hash}; use core::iter::{Chain, FusedIterator}; use core::ops::{BitAnd, BitAndAssign, BitOr, BitOrAssign, BitXor, BitXorAssign, Sub, SubAssign}; @@ -911,45 +910,16 @@ where /// ``` #[cfg_attr(feature = "inline-more", inline)] pub fn get_or_insert(&mut self, value: T) -> &T { - // Although the raw entry gives us `&mut T`, we only return `&T` to be consistent with - // `get`. Key mutation is "raw" because you're not supposed to affect `Eq` or `Hash`. - self.map - .raw_entry_mut() - .from_key(&value) - .or_insert(value, ()) - .0 - } - - /// Inserts an owned copy of the given `value` into the set if it is not - /// present, then returns a reference to the value in the set. - /// - /// # Examples - /// - /// ``` - /// use hashbrown::HashSet; - /// - /// let mut set: HashSet = ["cat", "dog", "horse"] - /// .iter().map(|&pet| pet.to_owned()).collect(); - /// - /// assert_eq!(set.len(), 3); - /// for &pet in &["cat", "dog", "fish"] { - /// let value = set.get_or_insert_owned(pet); - /// assert_eq!(value, pet); - /// } - /// assert_eq!(set.len(), 4); // a new "fish" was inserted - /// ``` - #[inline] - pub fn get_or_insert_owned(&mut self, value: &Q) -> &T - where - Q: Hash + Equivalent + ToOwned + ?Sized, - { - // Although the raw entry gives us `&mut T`, we only return `&T` to be consistent with - // `get`. Key mutation is "raw" because you're not supposed to affect `Eq` or `Hash`. - self.map - .raw_entry_mut() - .from_key(value) - .or_insert_with(|| (value.to_owned(), ())) - .0 + let hash = make_hash(&self.map.hash_builder, &value); + let bucket = match self.map.table.find_or_find_insert_slot( + hash, + equivalent_key(&value), + make_hasher(&self.map.hash_builder), + ) { + Ok(bucket) => bucket, + Err(slot) => unsafe { self.map.table.insert_in_slot(hash, slot, (value, ())) }, + }; + unsafe { &bucket.as_ref().0 } } /// Inserts a value computed from `f` into the set if the given `value` is @@ -970,19 +940,33 @@ where /// } /// assert_eq!(set.len(), 4); // a new "fish" was inserted /// ``` + /// + /// The following example will panic because the new value doesn't match. + /// + /// ```should_panic + /// let mut set = hashbrown::HashSet::new(); + /// set.get_or_insert_with("rust", |_| String::new()); + /// ``` #[cfg_attr(feature = "inline-more", inline)] pub fn get_or_insert_with(&mut self, value: &Q, f: F) -> &T where Q: Hash + Equivalent + ?Sized, F: FnOnce(&Q) -> T, { - // Although the raw entry gives us `&mut T`, we only return `&T` to be consistent with - // `get`. Key mutation is "raw" because you're not supposed to affect `Eq` or `Hash`. - self.map - .raw_entry_mut() - .from_key(value) - .or_insert_with(|| (f(value), ())) - .0 + let hash = make_hash(&self.map.hash_builder, &value); + let bucket = match self.map.table.find_or_find_insert_slot( + hash, + equivalent_key(value), + make_hasher(&self.map.hash_builder), + ) { + Ok(bucket) => bucket, + Err(slot) => { + let new = f(value); + assert!(value.equivalent(&new), "new value is not equivalent"); + unsafe { self.map.table.insert_in_slot(hash, slot, (new, ())) } + } + }; + unsafe { &bucket.as_ref().0 } } /// Gets the given value's corresponding entry in the set for in-place manipulation. @@ -1607,15 +1591,21 @@ where /// ``` fn bitxor_assign(&mut self, rhs: &HashSet) { for item in rhs { - let entry = self.map.raw_entry_mut().from_key(item); - match entry { - map::RawEntryMut::Occupied(e) => { - e.remove(); - } - map::RawEntryMut::Vacant(e) => { - e.insert(item.to_owned(), ()); - } - }; + let hash = make_hash(&self.map.hash_builder, &item); + match self.map.table.find_or_find_insert_slot( + hash, + equivalent_key(item), + make_hasher(&self.map.hash_builder), + ) { + Ok(bucket) => unsafe { + self.map.table.remove(bucket); + }, + Err(slot) => unsafe { + self.map + .table + .insert_in_slot(hash, slot, (item.clone(), ())); + }, + } } } }