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

Euclideanring overflow #30

Closed

Conversation

jacereda
Copy link
Contributor

@jacereda jacereda commented Sep 3, 2017

No description provided.

@garyb
Copy link
Member

garyb commented Sep 22, 2017

This looks like it's about right to me, @hdgarrood what do you think? (I'm not really sure if saying that overflow = success is a safe assumption)

(Sorry about the delays on these @jacereda, will get these enum things in ASAP now - thanks very much for working on them).

@hdgarrood
Copy link
Contributor

Yeah I agree, this assumption seems fine for Int but it’s not really clear to me that it makes sense for a general Euclidean ring. I think this could easily mask genuine errors for types where multiplication or division is harder to implement correctly, for instance.

I think the answer is really to use a newtype which only generates pairs of values which do not overflow when multiplied. Maybe problems like this are really a sign that the quickcheck approach of using a standard generator for any type is not the best approach here.

@jacereda
Copy link
Contributor Author

I guess it isn't perfect, but isn't failing the test worse?

@hdgarrood
Copy link
Contributor

No, I think a false negative (tests pass and miss an error they should have caught) is worse than a false positive (tests fail due to overflow which we were expecting anyway), especially since we can address the latter using a newtype.

Base automatically changed from master to main February 27, 2021 02:04
@JordanMartinez
Copy link
Contributor

Looks like the next action for this PR is to drop the overflow check and instead wrap Int in a newtype that limits how large the initial generated Int can be, so that an overflow error cannot occur. That would fix the false positive.

@JordanMartinez
Copy link
Contributor

Superceded by #58

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

Successfully merging this pull request may close these issues.

4 participants