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

fix Issue 21834 - std.numeric.gcd can't handle negative values #7974

Merged
merged 1 commit into from
Apr 17, 2021

Conversation

ibuclaw
Copy link
Member

@ibuclaw ibuclaw commented Apr 16, 2021

This is a pretty lame restriction. Parameters should always be transformed to an unsigned value.

@ibuclaw ibuclaw added the math label Apr 16, 2021
@ibuclaw ibuclaw requested a review from andralex as a code owner April 16, 2021 23:59
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @ibuclaw!

Bugzilla references

Auto-close Bugzilla Severity Description
21834 normal std.numeric.gcd can't handle negative values

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7974"

@ibuclaw ibuclaw force-pushed the issue21834 branch 3 times, most recently from 78398a8 to af58e8c Compare April 17, 2021 01:27
@ibuclaw
Copy link
Member Author

ibuclaw commented Apr 17, 2021

Eh... you can't mix signed and unsigned types as parameters for gcd, that seems like another oversight.

@thewilsonator
Copy link
Contributor

please merge this when you're satisfied with it.

@ibuclaw
Copy link
Member Author

ibuclaw commented Apr 17, 2021

please merge this when you're satisfied with it.

If you're happy with the new signature...

On reviewing this morning, I'd feel more sure if there was a mixed types test added to verify it is handled correctly.

I'll have a small dig around the git history as well to add a few more comments, as why DigitalMars uses Euclid's algorithm is beyond me.

std/numeric.d Outdated Show resolved Hide resolved
@ibuclaw
Copy link
Member Author

ibuclaw commented Apr 17, 2021

@thewilsonator @berni44 - rebased on top of other PR refactoring/fixes and added more tests.

@berni44 berni44 merged commit 0b76336 into dlang:master Apr 17, 2021
@ibuclaw ibuclaw deleted the issue21834 branch April 17, 2021 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants