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

Creating invalid InclusiveInterval s #53

Closed
stefnotch opened this issue Dec 27, 2023 · 4 comments · Fixed by #56
Closed

Creating invalid InclusiveInterval s #53

stefnotch opened this issue Dec 27, 2023 · 4 comments · Fixed by #56

Comments

@stefnotch
Copy link

This crate requires InclusiveRanges to be valid when passing them to any of the functions, otherwise a panic will happen

	/// Is the range is valid, which according to this crate means `start()`
	/// <= `end()`
	fn is_valid(&self) -> bool
	where
		I: PointType,
	{
		self.start() <= self.end()
	}

Would it make sense to verify that in the constructor of InclusiveInterval? Then the implementation of InclusiveRange for InclusiveInterval could have is_valid be a no-op, and errors might be caught slightly sooner.

@ripytide
Copy link
Owner

	//only return valid range_bounds
	return CutResult {
		before_cut: result.before_cut.filter(|x| is_valid_range(*x)),
		inside_cut: result.inside_cut.filter(|x| is_valid_range(*x)),
		after_cut: result.after_cut.filter(|x| is_valid_range(*x)),
	};

In the implementation I sometimes create invalid range and then filter them out at the end as is makes some of the logic simpler but I would be fine with creating an unchecked variant of the constructor for my internal use. Catching invalid ranges earlier will be a great improvement, thanks.

@ripytide
Copy link
Owner

Hmm, I forgot but when going to implement this I remembered that if we ensure validity upon creation of a InclusiveInterval then we also have to make the start and end struct members private. This would mean you would no longer be able to write inclusive_interval.start and we would have to redirect it into a getter such as inclusive_interval.start(). This may still be an acceptable tradeoff, I'm not sure

@ripytide
Copy link
Owner

Hmm I think it would still be an acceptable tradeoff as I can still use the .start .end inside the implementation just fine and it would prevent all the panic sections on all the methods.

@ripytide
Copy link
Owner

ripytide commented Dec 27, 2023

Another caveat discovered upon implementation is that the panics on all the methods will still be necessary as on a custom range type the user can still create an invalid range. But I can continue the invariant constructors way on the InclusiveInterval struct.

@ripytide ripytide linked a pull request Dec 27, 2023 that will close this issue
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 a pull request may close this issue.

2 participants