feat: Implement SmallSet - #7
Conversation
Implement SmallSet as a wrapper around SmallMap where the value is (), following the same pattern as hashbrown's HashSet.
There was a problem hiding this comment.
Pull request overview
This PR implements SmallSet as a wrapper around SmallMap where the value is (), following the same pattern as hashbrown's HashSet. The implementation provides a complete set API with inline SIMD-accelerated storage for small collections that automatically spills to heap when exceeding the inline capacity.
Changes:
- Added
SmallSetstruct and complete API implementation insrc/set.rswith comprehensive documentation and tests - Added type aliases (
RapidSmallSet,FxSmallSet,ASmallSet) and module exports insrc/lib.rs - Added
Cloneimplementation forKeysiterator to supportSmallSet::Itercloning - Updated README with SmallSet usage examples and documentation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/set.rs | Complete SmallSet implementation with 929 lines including struct definition, API methods, iterators, trait implementations, and comprehensive unit tests |
| src/lib.rs | Module integration, type alias definitions for different hashers, and Clone impl for Keys iterator |
| README.md | Added SmallSet section with usage examples and explanation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// `heap_hasher` to hash the keys. | ||
| /// | ||
| /// The set will be able to hold at least `capacity` elements without | ||
| /// reallocating. If `capacity` is 0, the set will not allocate. |
There was a problem hiding this comment.
The documentation states "If capacity is 0, the set will not allocate" but this is incorrect. Based on the SmallMap implementation (src/lib.rs:298) and the behavior of with_capacity_and_hashers, the correct statement should be "If capacity is smaller than or equal to N, the set will not allocate" to match the actual behavior where capacity is compared against N to decide between inline and heap storage.
| /// reallocating. If `capacity` is 0, the set will not allocate. | |
| /// reallocating. If `capacity` is smaller than or equal to `N`, the set will not allocate. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// normally only possible through [`Cell`], [`RefCell`], global state, I/O, or | ||
| /// unsafe code. | ||
| /// | ||
| /// It is also a logic error for the [`Hash`] implementation of a key to panic. |
There was a problem hiding this comment.
The documentation refers to "the [Hash] implementation of a key" but this is a set, not a map. It should say "element" or "item" instead of "key" for consistency with set terminology.
| /// It is also a logic error for the [`Hash`] implementation of a key to panic. | |
| /// It is also a logic error for the [`Hash`] implementation of an element to panic. |
| } | ||
|
|
||
| /// Creates an empty `SmallSet` with the specified capacity, using | ||
| /// `heap_hasher` to hash the keys. |
There was a problem hiding this comment.
The documentation says "to hash the keys" but this is a set, not a map. It should say "to hash the elements" or "to hash the values" for consistency with set terminology.
| /// `heap_hasher` to hash the keys. | |
| /// `heap_hasher` to hash the elements. |
| } | ||
|
|
||
| /// Creates an empty `SmallSet` which will use the given hash builders to hash | ||
| /// keys. |
There was a problem hiding this comment.
The documentation says "to hash keys" but this is a set, not a map. It should say "to hash elements" or "to hash values" for consistency with set terminology.
| /// keys. | |
| /// elements. |
| SmallSet<N, T, SH, SI, LINEAR_THRESHOLD> | ||
| { | ||
| /// Creates an empty `SmallSet` which will use the given hash builder to hash | ||
| /// keys. |
There was a problem hiding this comment.
The documentation says "to hash keys" but this is a set, not a map. It should say "to hash elements" or "to hash values" for consistency with set terminology.
| /// keys. | |
| /// elements. |
| /// Creates an empty `SmallSet` with the specified capacity. | ||
| /// | ||
| /// The set will be able to hold at least `capacity` elements without | ||
| /// reallocating. If `capacity` is smaller than N, the set will not allocate. |
There was a problem hiding this comment.
The documentation states "If capacity is smaller than N, the set will not allocate." However, looking at the implementation in SmallMap::with_capacity, it checks if capacity > N to decide whether to allocate. This means the set will not allocate when capacity <= N, not just when capacity < N. The documentation should say "If capacity is at most N" or "If capacity is N or less" for accuracy.
| /// reallocating. If `capacity` is smaller than N, the set will not allocate. | |
| /// reallocating. If `capacity` is at most `N`, the set will not allocate. |
| /// `heap_hasher` for heap storage and `inline_hasher` for inline storage. | ||
| /// | ||
| /// The set will be able to hold at least `capacity` elements without | ||
| /// reallocating. If `capacity` is smaller than N, the set will not allocate. |
There was a problem hiding this comment.
The documentation states "If capacity is smaller than N, the set will not allocate." This is inconsistent with line 273 which correctly states "smaller than or eq to N". The implementation checks capacity > N, so the set will not allocate when capacity <= N. For consistency and accuracy, this should match line 273 and say "If capacity is at most N" or "If capacity is N or less".
| /// reallocating. If `capacity` is smaller than N, the set will not allocate. | |
| /// reallocating. If `capacity` is at most N, the set will not allocate. |
| /// `heap_hasher` to hash the keys. | ||
| /// | ||
| /// The set will be able to hold at least `capacity` elements without | ||
| /// reallocating. If `capacity` is smaller than or eq to N, the hash map will not allocate. |
There was a problem hiding this comment.
The documentation refers to "the hash map" but this is a set, not a map. It should say "the set will not allocate" for consistency.
| /// reallocating. If `capacity` is smaller than or eq to N, the hash map will not allocate. | |
| /// reallocating. If `capacity` is smaller than or eq to N, the set will not allocate. |
| /// k1 == k2 -> hash(k1) == hash(k2) | ||
| /// ``` | ||
| /// | ||
| /// In other words, if two keys are equal, their hashes must be equal. |
There was a problem hiding this comment.
The documentation refers to "two keys" but this is a set, not a map. It should say "if two elements are equal" or "if two values are equal" for clarity and consistency with set terminology.
| /// In other words, if two keys are equal, their hashes must be equal. | |
| /// In other words, if two elements are equal, their hashes must be equal. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces SmallSet as a wrapper around SmallMap, which is a great addition for feature parity with std and hashbrown. The implementation is solid, following the established pattern of building a set on top of a map. The code is well-documented and includes a comprehensive test suite, which is excellent. I have a couple of minor suggestions: one to fix a typo in a doc comment and another to make the replace method's implementation slightly more direct. Overall, this is a high-quality contribution.
| /// `heap_hasher` to hash the elements. | ||
| /// | ||
| /// The set will be able to hold at least `capacity` elements without | ||
| /// reallocating. If `capacity` is smaller than or eq to N, the hash map will not allocate. |
There was a problem hiding this comment.
There's a small typo in the documentation. It should refer to set instead of hash map.
| /// reallocating. If `capacity` is smaller than or eq to N, the hash map will not allocate. | |
| /// reallocating. If `capacity` is smaller than or eq to N, the set will not allocate. |
| pub fn replace(&mut self, value: T) -> Option<T> { | ||
| // Try to remove the old value first | ||
| let old_value = self.take(&value); | ||
| // Insert the new value | ||
| self.insert(value); | ||
| old_value | ||
| } |
There was a problem hiding this comment.
The current implementation of replace is correct, but it could be made more direct by operating on self.map instead of calling other SmallSet methods like take and insert. This makes the logic slightly clearer and avoids ignoring the bool return value of self.insert.
This approach, which involves two lookups (one for removal, one for insertion), is consistent with hashbrown::HashSet's implementation and is a reasonable approach given that SmallMap does not expose an entry-like API for single-lookup updates.
| pub fn replace(&mut self, value: T) -> Option<T> { | |
| // Try to remove the old value first | |
| let old_value = self.take(&value); | |
| // Insert the new value | |
| self.insert(value); | |
| old_value | |
| } | |
| pub fn replace(&mut self, value: T) -> Option<T> { | |
| // To get the old value, we must remove it first, then insert the new one. | |
| // This is what `hashbrown::HashSet::replace` does. | |
| let old = self.map.remove_entry(&value); | |
| self.map.insert(value, ()); | |
| old.map(|(k, _)| k) | |
| } |
ee88359 to
ded6990
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Implement SmallSet as a wrapper around SmallMap where the value is
(), following the same pattern as hashbrown's HashSet.Note: Much of this implementation involves boilerplate delegation to SmallMap. These sections were copilot-generated, but have been manually reviewed. I have added a robust set of unit tests to validate the behavior.