Skip to content
This repository was archived by the owner on Sep 1, 2020. It is now read-only.

Conversation

soc
Copy link

@soc soc commented Sep 6, 2014

... between integral and floating point types.

soc added 3 commits September 6, 2014 22:11
... subtyping relationship between integral and floating-point types.
@soc soc changed the title Topic/no implicit widening conversions between intergral and floating point types No implicit widening conversions ... Sep 6, 2014
@non
Copy link

non commented Sep 6, 2014

I think we'd want to put this behind a -Z flag or a language import to allow for source compatibility. Do you think that would be possible to achieve?

As you expect, I am a big fan of this change, so assuming the compatibility concerns are addressed I would definitely vote for its inclusion.

@soc
Copy link
Author

soc commented Sep 6, 2014

Probably, but adding configurability will probably 10 times more complicated than the fix itself.
One would have to duplicate all places I touched to have both the old and the new logic (instead of replacing it), plus the lookup whether the flag/language import is enabled/in scope.
Additionally, one would have to figure out a way to make those implicit defs "disappear" depending on whether the language import is in scope.

@soc
Copy link
Author

soc commented Sep 6, 2014

Note that there is still an issue remaining in one of the tests. (But I guess it won't matter currently, because we don't have a build bot yet.)

@propensive
Copy link

@soc See #31.

@@ -57,7 +57,7 @@ trait Constants extends api.Constants {
def isCharRange: Boolean = isIntRange && Char.MinValue <= intValue && intValue <= Char.MaxValue
def isIntRange: Boolean = ByteTag <= tag && tag <= IntTag
def isLongRange: Boolean = ByteTag <= tag && tag <= LongTag
def isFloatRange: Boolean = ByteTag <= tag && tag <= FloatTag
def isDoubleRange: Boolean= FloatTag == tag || tag == DoubleTag
def isNumeric: Boolean = ByteTag <= tag && tag <= DoubleTag
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what's going on here?

Copy link

Choose a reason for hiding this comment

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

This is saying that only floats and doubles fit in doubles, instead of all the numeric types. Then it renames the method. Before there wasn't an "isDoubleRange" because in the context this was being used, that was always true. And afterward I suppose there's no "isFloatRange" because it would only be true for floats and thus never checked.

@milessabin
Copy link
Member

I agree with @non on this. We want it to be possible for existing code to be compiled with tlc with at most the addition of a compiler switch ... so working out how to do that cleanly isn't an optional extra, it's the most important thing.

@@ -467,9 +467,5 @@ object Long extends AnyValCompanion {

/** The String representation of the scala.Long companion object. */
override def toString = "object scala.Long"
/** Language mandated coercions from Long to "wider" types. */
Copy link

Choose a reason for hiding this comment

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

Nice comment paulp.

@paulp
Copy link

paulp commented Sep 8, 2014

If you proceed with this or something like it you might want to give https://issues.scala-lang.org/browse/SI-8338 a look.

@soc
Copy link
Author

soc commented Sep 8, 2014

See #34.

@soc
Copy link
Author

soc commented Sep 8, 2014

@milessabin Yes, I'm aware of the issue.

I only complain about the pain of implementing a flag, not the need to add it.

@typelevel-bot
Copy link

Can one of the admins verify this patch?

@soc soc closed this Sep 25, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants