Skip to content

Commit

Permalink
Auto merge of #555 - cuviper:set-no-raw_entry, r=Amanieu
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bors committed Sep 18, 2024
2 parents 3b350d7 + 6420cfd commit 67f704e
Show file tree
Hide file tree
Showing 3 changed files with 109 additions and 70 deletions.
4 changes: 2 additions & 2 deletions ci/run.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
22 changes: 17 additions & 5 deletions src/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1741,11 +1741,7 @@ where
#[cfg_attr(feature = "inline-more", inline)]
pub fn insert(&mut self, k: K, v: V) -> Option<V> {
let hash = make_hash::<K, S>(&self.hash_builder, &k);
let hasher = make_hasher::<_, V, S>(&self.hash_builder);
match self
.table
.find_or_find_insert_slot(hash, equivalent_key(&k), hasher)
{
match self.find_or_find_insert_slot(hash, &k) {
Ok(bucket) => Some(mem::replace(unsafe { &mut bucket.as_mut().1 }, v)),
Err(slot) => {
unsafe {
Expand All @@ -1756,6 +1752,22 @@ where
}
}

#[cfg_attr(feature = "inline-more", inline)]
pub(crate) fn find_or_find_insert_slot<Q>(
&mut self,
hash: u64,
key: &Q,
) -> Result<Bucket<(K, V)>, crate::raw::InsertSlot>
where
Q: Equivalent<K> + ?Sized,
{
self.table.find_or_find_insert_slot(
hash,
equivalent_key(key),
make_hasher(&self.hash_builder),
)
}

/// Insert a key-value pair into the map without checking
/// if the key already exists in the map.
///
Expand Down
153 changes: 90 additions & 63 deletions src/set.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
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};
use core::{fmt, mem};
use map::{equivalent_key, make_hash, make_hasher};
use map::make_hash;

use super::map::{self, HashMap, Keys};
use crate::raw::{Allocator, Global, RawExtractIf};
Expand Down Expand Up @@ -911,45 +910,12 @@ 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<String> = ["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<Q>(&mut self, value: &Q) -> &T
where
Q: Hash + Equivalent<T> + ToOwned<Owned = T> + ?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.find_or_find_insert_slot(hash, &value) {
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
Expand All @@ -970,19 +936,29 @@ 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<Q, F>(&mut self, value: &Q, f: F) -> &T
where
Q: Hash + Equivalent<T> + ?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.find_or_find_insert_slot(hash, value) {
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.
Expand Down Expand Up @@ -1157,11 +1133,7 @@ where
#[cfg_attr(feature = "inline-more", inline)]
pub fn replace(&mut self, value: T) -> Option<T> {
let hash = make_hash(&self.map.hash_builder, &value);
match self.map.table.find_or_find_insert_slot(
hash,
equivalent_key(&value),
make_hasher(&self.map.hash_builder),
) {
match self.map.find_or_find_insert_slot(hash, &value) {
Ok(bucket) => Some(mem::replace(unsafe { &mut bucket.as_mut().0 }, value)),
Err(slot) => {
unsafe {
Expand Down Expand Up @@ -1607,15 +1579,17 @@ where
/// ```
fn bitxor_assign(&mut self, rhs: &HashSet<T, S, A>) {
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.find_or_find_insert_slot(hash, item) {
Ok(bucket) => unsafe {
self.map.table.remove(bucket);
},
Err(slot) => unsafe {
self.map
.table
.insert_in_slot(hash, slot, (item.clone(), ()));
},
}
}
}
}
Expand Down Expand Up @@ -2598,7 +2572,7 @@ fn assert_covariance() {

#[cfg(test)]
mod test_set {
use super::HashSet;
use super::{make_hash, Equivalent, HashSet};
use crate::DefaultHashBuilder;
use std::vec::Vec;

Expand Down Expand Up @@ -3072,4 +3046,57 @@ mod test_set {
assert_eq!(HashSet::<u32>::new().allocation_size(), 0);
assert!(HashSet::<u32>::with_capacity(1).allocation_size() > core::mem::size_of::<u32>());
}

#[test]
fn duplicate_insert() {
let mut set = HashSet::new();
set.insert(1);
set.get_or_insert_with(&1, |_| 1);
set.get_or_insert_with(&1, |_| 1);
assert!([1].iter().eq(set.iter()));
}

#[test]
#[should_panic]
fn some_invalid_equivalent() {
use core::hash::{Hash, Hasher};
struct Invalid {
count: u32,
other: u32,
}

struct InvalidRef {
count: u32,
other: u32,
}

impl PartialEq for Invalid {
fn eq(&self, other: &Self) -> bool {
self.count == other.count && self.other == other.other
}
}
impl Eq for Invalid {}

impl Equivalent<Invalid> for InvalidRef {
fn equivalent(&self, key: &Invalid) -> bool {
self.count == key.count && self.other == key.other
}
}
impl Hash for Invalid {
fn hash<H: Hasher>(&self, state: &mut H) {
self.count.hash(state);
}
}
impl Hash for InvalidRef {
fn hash<H: Hasher>(&self, state: &mut H) {
self.count.hash(state);
}
}
let mut set: HashSet<Invalid> = HashSet::new();
let key = InvalidRef { count: 1, other: 1 };
let value = Invalid { count: 1, other: 2 };
if make_hash(set.hasher(), &key) == make_hash(set.hasher(), &value) {
set.get_or_insert_with(&key, |_| value);
}
}
}

0 comments on commit 67f704e

Please sign in to comment.