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

refactor: Replace the bisection dependency #18

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

musicinmybrain
Copy link

@musicinmybrain musicinmybrain commented May 9, 2024

The bisection crate appears unmaintained (last commit four years ago), and has a couple of unaddressed issues. Since Rust 1.52, we can rely on the standard library and use partition_point instead.

Comparing the definition of partition_point with the definitions of bisect_left and bisect_right, I believe this replacement should be exactly equivalent. However, I don’t believe the test coverage is sufficient to catch any subtle errors here, so I recommend closely double-checking my work.

@musicinmybrain musicinmybrain changed the title Replace the bisection dependency refactor: Replace the bisection dependency May 9, 2024
@baszalmstra
Copy link
Collaborator

Awesome! The less dependencies the better.

What do you think is missing with regard to test coverage? Maybe you could add that?

@musicinmybrain
Copy link
Author

Awesome! The less dependencies the better.

What do you think is missing with regard to test coverage? Maybe you could add that?

I’m just not really confident that the coverage of various stages of the SparseRange is thorough enough to cover any subtle mistakes here, which is why I say I’m mostly relying on a careful reading of the relevant documentation. I’m not sure how the coverage could be very straightforwardly improved.

For example, if I incorrectly write < instead of <= in let right_index = self.left.partition_point(|x| x <= &(range_end + 1)); on line 58, the tests still pass. (If I make the same intentional error on line 100 instead, I do get a test failure.) But what test case should I add to cover that error? I guess with some careful analysis, and after developing a better global understanding of the code, I could derive a state in which that particular error causes the wrong outcome. But what about the kinds of errors I haven’t thought of?

This is the kind of situation where some people would reach for property-based testing, like Python’s hypothesis, but there doesn’t seem to be an actively-maintained crate for that in the Rust ecosystem at the moment.

@baszalmstra
Copy link
Collaborator

https://crates.io/crates/proptest seems pretty well maintained

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.

None yet

2 participants