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 #15489, don't check conversions during mixed signedness arithmetic #23827

Merged
merged 1 commit into from Oct 10, 2017

Conversation

JeffBezanson
Copy link
Sponsor Member

also skip checks for integer bitwise ops

base/int.jl Outdated
T = promote_typeof(a, b)
return $op(a % T, b % T)
end
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Wouldn't these definitions work for Integer as well? Would collapse all of these definitions.

@StefanKarpinski
Copy link
Sponsor Member

The basic observation here is that (a + b) % n == (a % n) + (b % n) % n for any a, b, n, so whatever the modulus of the target integer type, we can mod the original arguments into that, then do overflowing integer operations in that modulus and get the same result as if we'd done the operations in arbitrary precision and then reduced to the modulus of the target type. None of that depends on the signedness of the arguments, so we can do this completely universally for all kinds of integers – as long as they implement some kind of modular arithmetic.

@JeffBezanson
Copy link
Sponsor Member Author

That's right. But I was worried about integer types that might implement convert and promote_rule, but not %. Maybe we should add a fallback % that calls convert?

@StefanKarpinski
Copy link
Sponsor Member

The argument works for - and * for all n and for bitwise ops when n is a power of 2.

@StefanKarpinski
Copy link
Sponsor Member

Beautiful! 👏

@JeffBezanson
Copy link
Sponsor Member Author

Failure on 64-bit AV might be a variation of #23371?

@StefanKarpinski
Copy link
Sponsor Member

I don't think these failures are related, but I really wish we could get a clean CI run here.

@StefanKarpinski
Copy link
Sponsor Member

Ah, this feels very nice. I'm very pleased with how this turned out.

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