rolling-hash #192
rolling-hash #192
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the RollingHash implementation to support custom base values through const generics, and replaces the is_substring method with more flexible substring and position methods.
- Converts
RollingHashto a generic struct with a constBASEparameter, allowing different hash bases - Introduces a
Substringtype to represent extracted substrings with their hash values - Replaces the boolean
is_substringmethod withpositionthat returns the index of the first occurrence
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libs/rolling_hash/src/lib.rs
Outdated
| let mut pows = vec![1; n + 1]; | ||
| for (i, &x) in xs.iter().enumerate() { | ||
| // hashes[i + 1] = hashes[i] * BASE + x | ||
| // hashes[i + 1] = hashes[i] * base + x |
There was a problem hiding this comment.
[nitpick] The comment uses lowercase "base" but the actual constant is uppercase "BASE". Consider updating the comment to match: "hashes[i + 1] = hashes[i] * BASE + x" for consistency with the code.
| // hashes[i + 1] = hashes[i] * base + x | |
| // hashes[i + 1] = hashes[i] * BASE + x |
| pub fn substring(&self, range: ops::Range<usize>) -> Substring<BASE> { | ||
| let len = range.end - range.start; | ||
| let hash = self.hash(range); | ||
| Substring::new(hash, len) | ||
| } |
There was a problem hiding this comment.
The substring method lacks documentation. Consider adding a doc comment explaining what this method does, its parameters, and what it returns. For example: /// Creates a Substring from the specified range of this RollingHash.
| pub fn position(&self, sub: &Substring<BASE>) -> Option<usize> { | ||
| if sub.len > self.len() { | ||
| return None; | ||
| } | ||
| false | ||
| (0..=self.len() - sub.len).find(|&i| self.hash(i..(i + sub.len)) == sub.hash) | ||
| } |
There was a problem hiding this comment.
The position method lacks documentation. Consider adding a doc comment explaining what this method does, its parameters, return value (e.g., returns the first position where the substring is found, or None if not found), and complexity. For example: /// Finds the first position of the substring in this RollingHash. Returns None if not found. O(n) where n is the length of this RollingHash.
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | ||
| pub struct Substring<const BASE: u64> { | ||
| hash: u64, | ||
| len: usize, | ||
| } |
There was a problem hiding this comment.
The Substring struct lacks documentation. Consider adding a doc comment explaining what this struct represents and its purpose. For example: /// Represents a substring with its precomputed hash value and length.
| pub fn is_empty(&self) -> bool { | ||
| self.xs.is_empty() | ||
| } | ||
|
|
There was a problem hiding this comment.
The base method lacks documentation. Consider adding a doc comment explaining what this method returns. For example: /// Returns the base value used for this rolling hash.
| /// Returns the base value used for this rolling hash. |
libs/rolling_hash/src/lib.rs
Outdated
| // hashes[i + 1] = hashes[i] * base + x | ||
| hashes[i + 1] = calc_mod(mul(hashes[i], BASE) + x); | ||
| // pows[i + 1] = pows[i] * BASE | ||
| // pows[i + 1] = pows[i] * base |
There was a problem hiding this comment.
[nitpick] The comment uses lowercase "base" but the actual constant is uppercase "BASE". Consider updating the comment to match: "pows[i + 1] = pows[i] * BASE" for consistency with the code.
No description provided.