Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add an hasSize function #7

Merged
merged 5 commits into from
Aug 15, 2024
Merged

Conversation

alberic89
Copy link
Contributor

No description provided.

src/root.zig Outdated
@@ -210,6 +210,15 @@ pub fn AlgorithmType(
self.max_needle = max_needle;
}

// Check is there is enough memory allocated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Check is there is enough memory allocated
// Check if buffers have sufficient memory for a given haystack and
// needle length.

src/root.zig Outdated
Comment on lines 215 to 219
if (self.max_haystack < max_haystack or self.max_needle < max_needle) {
return false;
} else {
return true;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (self.max_haystack < max_haystack or self.max_needle < max_needle) {
return false;
} else {
return true;
}
return (max_haystack <= self.max_haystack) and (max_needle <= self.max_needle);

It's easier just to check the positive case.

src/root.zig Outdated
@@ -210,6 +210,15 @@ pub fn AlgorithmType(
self.max_needle = max_needle;
}

// Check is there is enough memory allocated
pub fn hasSize(self: *Self, max_haystack: usize, max_needle: usize) bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn hasSize(self: *Self, max_haystack: usize, max_needle: usize) bool {
pub fn hasSize(self: *const Self, max_haystack: usize, max_needle: usize) bool {

This won't and should not mutate memory, so we mark the pointer as const.

src/root.zig Outdated
Comment on lines 662 to 663
// Check is there is enough memory allocated
pub fn hasSize(self: *Ascii, max_haystack: usize, max_needle: usize) bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Check is there is enough memory allocated
pub fn hasSize(self: *Ascii, max_haystack: usize, max_needle: usize) bool {
// Check if buffers have sufficient memory for a given haystack and
// needle length.
pub fn hasSize(self: *const Ascii, max_haystack: usize, max_needle: usize) bool {

src/unicode.zig Outdated
Comment on lines 223 to 224
// Check is there is enough memory allocated
pub fn hasSize(self: *Unicode, max_haystack: usize, max_needle: usize) bool {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Check is there is enough memory allocated
pub fn hasSize(self: *Unicode, max_haystack: usize, max_needle: usize) bool {
// Check if buffers have sufficient memory for a given haystack and
// needle length.
pub fn hasSize(self: *const Unicode, max_haystack: usize, max_needle: usize) bool {

@fjebaker
Copy link
Owner

fjebaker commented Aug 14, 2024

In your previous PR you had some tests that made sure resize works. It would be very useful to include those (and similar for hasSize) in this PR too 👍

@alberic89
Copy link
Contributor Author

But just a question : is it an error that there is < and not <= at

std.debug.assert(needle.len < self.maximumNeedleLen());
?
It makes score fail when there is just the size needed.

@fjebaker
Copy link
Owner

fjebaker commented Aug 14, 2024

is it an error that there is < and not <= at

Yes that is a mistake. Well spotted! Make sure there's a test that catches it and then I think this will be ready to merge.

Edit: I see you're ahead of me and already have!

Copy link
Owner

@fjebaker fjebaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is all great! My only suggestion is to add a handful more tests that check that smaller sizes pass, and that larger sizes do indeed fail (even after resize).

When you're happy, let me know and we can merge.

src/root.zig Show resolved Hide resolved
@alberic89
Copy link
Contributor Author

For me, all is good. We can merge it.

@fjebaker fjebaker merged commit 6204327 into fjebaker:main Aug 15, 2024
1 check passed
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.

2 participants