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

Integer /: restrict fallback to same-type case (fix #19714) #19779

Merged
merged 1 commit into from
Jan 3, 2017

Conversation

StefanKarpinski
Copy link
Member

The principle here is that if there's an implementation of a promoted operator for some type and promotions for that type, unless other methods are defined, the operation should always be "funnelled" through the core operator definition for that type. The method for /(::Integer, ::Integer) violated that principle since it would take precedence in mixed-integer-type division cases even in the presence of a same-type method definition for a custom integer type and the appropriate promotion rules.

@StefanKarpinski StefanKarpinski added this to the 0.6.0 milestone Dec 30, 2016
@tkelman
Copy link
Contributor

tkelman commented Dec 30, 2016

test case?

@StefanKarpinski
Copy link
Member Author

Now with 100% more test case.

@StefanKarpinski StefanKarpinski added the needs news A NEWS entry is required for this change label Dec 31, 2016
@vtjnash
Copy link
Member

vtjnash commented Dec 31, 2016

Not to be too pedantic, but if there were zero test cases before, 100% more test cases would still be zero. :trollface:

@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

In case this might have performance consequences (probably shouldn't but never know) @nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

Performance intuition is hard. Those regressions all look like mixed-type cases with BigInts, maybe a few specializations for BigInt could fix them?

@StefanKarpinski
Copy link
Member Author

StefanKarpinski commented Jan 2, 2017

The BigInt regression here makes sense, I think, since it constructs an extra BigInt before going to float/float division. I assume the complex division case is also due to the non-complex regression.

The principle here is that if there's an implementation of a promoted
operator for some type and promotions for that type, unless other
methods are defined, the operation should always be "funnelled"
through the core operator definition for that type. The method for
`/(::Integer, ::Integer)` violated that principle since it would take
precedence in mixed-integer-type division cases even in the presence
of a same-type method definition for a custom integer type and the
appropriate promotion rules.
@StefanKarpinski
Copy link
Member Author

@nanosoldier runbenchmarks(ALL, vs = ":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @jrevels

@KristofferC
Copy link
Member

That's nice.

@StefanKarpinski
Copy link
Member Author

I probably shouldn't have restarted that CI job since I think this is independent breakage.

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.

6 participants