Skip to content

Conversation

@AbeZbm
Copy link

@AbeZbm AbeZbm commented May 28, 2025

Hi,

Thanks for your time to review this PR.

By examining the existing code, we found that some tests can be added to improve the repo's overall test coverage. The tests we submitted have been carefully curated by us to ensure their behavior and effectiveness.
The code region we covered is:

strsim-rs/src/lib.rs

Lines 360 to 364 in 43d4443

if a_len == 0 {
return b_len;
}
if b_len == 0 {
return a_len;

strsim-rs/src/lib.rs

Lines 486 to 488 in 43d4443

if self.fill * 3 >= (self.mask + 1) * 2 {
self.grow((self.used + 1) * 2);
i = self.lookup(key);

strsim-rs/src/lib.rs

Lines 528 to 530 in 43d4443

}
perturb >>= 5;

strsim-rs/src/lib.rs

Lines 534 to 565 in 43d4443

fn grow(&mut self, min_used: i32) {
let mut new_size = self.mask + 1;
while new_size <= min_used {
new_size <<= 1;
}
self.fill = self.used;
self.mask = new_size - 1;
let old_map = std::mem::replace(
self.map
.as_mut()
.expect("callers have to ensure map is allocated"),
vec![GrowingHashmapMapElemChar::<ValueType>::default(); new_size as usize],
);
for elem in old_map {
if elem.value != Default::default() {
let j = self.lookup(elem.key);
let new_elem = &mut self.map.as_mut().expect("map created above")[j];
new_elem.key = elem.key;
new_elem.value = elem.value;
self.used -= 1;
if self.used == 0 {
break;
}
}
}
self.used = self.fill;
}
}

And I found that after creating a GrowingHashmapChar instance via GrowingHashmapChar::default(), the self.mask value is -1. If the grow() method is called directly without initializing the map (via allocate() or get_mut()), the following loop will run indefinitely:

strsim-rs/src/lib.rs

Lines 535 to 538 in 43d4443

let mut new_size = self.mask + 1;
while new_size <= min_used {
new_size <<= 1;
}

While current internal usage (e.g., get_mut() always initializes the map via allocate()) prevents this scenario, but it's never good that such situations exist. So I added a debug_assert for it.
Thanks again for reviewing.

@AbeZbm
Copy link
Author

AbeZbm commented May 30, 2025

@maxbachmann Hello! I was wondering if there's any feedback on this PR. I've ensured all checks are passing and I'm happy to address any concerns or make changes as needed. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant