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

Make all numeric coercions explicit. #2593

Merged
merged 1 commit into from May 28, 2013
Merged

Make all numeric coercions explicit. #2593

merged 1 commit into from May 28, 2013

Conversation

paulp
Copy link
Contributor

@paulp paulp commented May 24, 2013

Optimistically, this is preparation for a day when we don't let numeric types
drift with the winds. Even without the optimism it's a good idea. I found a
couple bugs in the process - how many people would see this one coming without
compiler help. One of these methods has an inappropriately wide return type.

def signum(x: Int): Int = java.lang.Integer.signum(x)
def signum(x: Long): Long = java.lang.Long.signum(x)
def signum(x: Float): Float = java.lang.Math.signum(x)
def signum(x: Double): Double = java.lang.Math.signum(x)

Managing type coercions manually is a bit tedious, no doubt, but it's not
tedious enough to warrant abandoning type safety just because java did it.

@soc
Copy link
Member

soc commented May 24, 2013

I agree in general, but the signum case is a pretty bad example. I have been working on related pieces of code and the huge issue is that we either need to decide

  • on one return type for signum, regardless of the input number type¹
  • on a return type which equals the input type

¹ Have a look at https://github.com/scala/scala/blob/master/src/library/scala/math/Numeric.scala#L205 where this goes terribly wrong, affecting all numeric types.

Even if it might create a minor performance hit to convert an Int to a Long the gained consistency is worth it, imho.

@soc
Copy link
Member

soc commented May 24, 2013

And when I said "terribly wrong" I don't meant "the numeric type drifts and it's annoying", but "we convert NaNs back to valid numbers"!

Compare Double.NaN.signum to math.signum(Double.NaN)

@paulp
Copy link
Contributor Author

paulp commented May 24, 2013

If it was intentional, it couldn't have been gone about in a worse way. If you want to bring consistency to something, bring some consistency and document what you did. Keeping the same name, the same overloads, and making an almost invisible change to the return type of one alternative -- and then not documenting that anything has changed from what is being forwarded -- is a better example of how to propagate inconsistency than how to limit it.

@soc
Copy link
Member

soc commented May 24, 2013

I totally agree. I think a comment should be added to explain why it is done.

@retronym
Copy link
Member

The overall patch LGTM, but I haven't followed the signum discussion closely enough to know if this patch is good to merge or not.

@paulp
Copy link
Contributor Author

paulp commented May 27, 2013

I don't have any feelings about signum, so I'll put it back as it was and comment that it's intentional.

Optimistically, this is preparation for a day when we don't
let numeric types drift with the winds. Even without the optimism
it's a good idea. It flushed out an undocumented change in
the math package object relative to the methods being forwarded (a
type is widened from what is returned in java) so I documented
the intentionality of it.

Managing type coercions manually is a bit tedious, no doubt,
but it's not tedious enough to warrant abandoning type safety
just because java did it.
@soc
Copy link
Member

soc commented May 28, 2013

LGTM

paulp added a commit that referenced this pull request May 28, 2013
Make all numeric coercions explicit.
@paulp paulp merged commit 6889cff into scala:master May 28, 2013
@paulp paulp deleted the pr/no-numeric-widen branch May 28, 2013 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants