several fixes to scala.concurrent.util.Duration #1261

Merged
merged 11 commits into from Sep 12, 2012

Projects

None yet

8 participants

@rkuhn
Contributor
rkuhn commented Sep 5, 2012
  • add test cases (migrated from Akka sources)
  • add overflow checking (will throw IllegalArgumentException instead of
    giving wrong results)
  • make string parsing more precise when giving >100days in nanoseconds
  • make method signatures more precise in retaining FiniteDuration
    throughout calculations
  • fix mul/div of infinities by negative number
  • add Ordering for Deadline (was accidentally left out earlier)
@rkuhn rkuhn several fixes to scala.concurrent.util.Duration
- add test cases (migrated from Akka sources)
- add overflow checking (will throw IllegalArgumentException instead of
  giving wrong results)
- make string parsing more precise when giving >100days in nanoseconds
- make method signatures more precise in retaining FiniteDuration
  throughout calculations
- fix mul/div of infinities by negative number
- add Ordering for Deadline (was accidentally left out earlier)
9da1358
@jsuereth
Member
jsuereth commented Sep 6, 2012

Reviewers?

@retronym retronym and 1 other commented on an outdated diff Sep 6, 2012
src/library/scala/concurrent/util/Duration.scala
@@ -289,12 +321,19 @@ class FiniteDuration(val length: Long, val unit: TimeUnit) extends Duration {
case x: FiniteDuration => toNanos compare x.toNanos
case _ => -(other compare this)
}
+
+ private def add(a: Long, b: Long): Long = {
+ val c = a + b
+ // check if the signs of the top bit of both summands differ from the sum
+ if (((a ^ c) & (b ^ c)) < 0) throw new IllegalArgumentException("integer overflow")
+ else c
@rkuhn
rkuhn Sep 6, 2012 Contributor

It took me about the same time to think through than the one above, but I can change it if you want. None of these are really dead-obvious ;-)

@retronym
retronym Sep 6, 2012 Member

I suggest to change it and add a comment that links to that CERT page.

@rkuhn
rkuhn Sep 6, 2012 Contributor

okay, will do

@rkuhn rkuhn second round of Duration cleanup
- make Duration behave consistent with Double's non-finite semantics
- add ScalaDoc
- add complete test suite
- change overflow protection impl after review comments
- clean up code
7a833f9
@rkuhn
Contributor
rkuhn commented Sep 7, 2012

PLS REBUILD ALL

@viktorklang viktorklang and 2 others commented on an outdated diff Sep 7, 2012
src/library/scala/concurrent/util/Duration.scala
timeUnit get unitName match {
- case Some(unit) => Duration(length, unit)
+ case Some(unit) =>
+ val valueStr = s1 dropRight unitName.length
+ val valueD = JDouble.parseDouble(valueStr)
+ if (valueD <= 9007199254740992d && valueD >= -9007199254740992d) Duration(valueD, unit)
@viktorklang
viktorklang Sep 7, 2012 Contributor

Magic number is magical. Pull out to a named constant and comment what it is for

@rkuhn
rkuhn Sep 7, 2012 Contributor

will do

@jsuereth
jsuereth Sep 7, 2012 Member

I hope you called it MAGIC_NUMBER, for clarity, and this test becomes: if(MAGIC_NUMBER is magic)

@viktorklang viktorklang and 1 other commented on an outdated diff Sep 7, 2012
src/library/scala/concurrent/util/Duration.scala
*/
val MinusInf: Infinite = new Infinite {
override def toString = "Duration.MinusInf"
- def compare(other: Duration) = if (other eq this) 0 else -1
+ def compare(other: Duration) = other match {
@viktorklang
viktorklang Sep 7, 2012 Contributor

Is this an improvement over old impl?

@rkuhn
rkuhn Sep 7, 2012 Contributor

no, there was one more case in the meantime and I didn't revert when that went away, will fix

@viktorklang viktorklang and 1 other commented on an outdated diff Sep 7, 2012
src/library/scala/concurrent/util/Duration.scala
+ * [[Duration.MinusInf]] and [[Duration.Undefined]], respectively.
+ */
+ def create(length: Double, unit: TimeUnit): Duration = apply(length, unit)
+ /**
+ * Construct a finite duration from the given length and time unit, where the latter is
+ * looked up in a list of string representation. Valid choices are:
+ *
+ * `d, day, h, hour, min, minute, s, sec, second, ms, milli, millisecond, µs, micro, microsecond, ns, nano, nanosecond`
+ * and their pluralized forms (for every but the first mentioned form of each unit, i.e. no "ds", but "days").
+ */
+ def create(length: Long, unit: String): FiniteDuration = apply(length, unit)
+ /**
+ * Parse String into Duration. Format is `"<length><unit>"`, where
+ * whitespace is allowed before, between and after the parts. Infinities are
+ * designated by `"Inf"`, `"PlusInf"`, `"+Inf"` and `"-Inf"` or `"MinusInf"`.
+ * Throws exception if format is not parseable.
@viktorklang
viktorklang Sep 7, 2012 Contributor

What kind of exception?

@rkuhn
rkuhn Sep 7, 2012 Contributor

good point

@viktorklang viktorklang commented on the diff Sep 7, 2012
src/library/scala/concurrent/util/Duration.scala
-
- def /(factor: Double) = fromNanos(toNanos.toDouble / factor)
-
- def /(other: Duration) = if (other.isFinite) toNanos.toDouble / other.toNanos else 0
+ def *(factor: Double) =
+ if (!factor.isInfinite) fromNanos(toNanos * factor)
+ else if (factor.isNaN) Undefined
+ else if ((factor > 0) ^ (this < Zero)) Inf
+ else MinusInf
+
+ def /(factor: Double) =
+ if (!factor.isInfinite) fromNanos(toNanos / factor)
+ else if (factor.isNaN) Undefined
+ else Zero
+
+ // if this is made a constant, then scalac will elide the conditional and always return +0.0, SI-6331
@viktorklang
viktorklang Sep 7, 2012 Contributor

@retronym said he'd take a stab at SI-6331 (for teh record)

@rkuhn
rkuhn Sep 7, 2012 Contributor

which is why I put this comment with the reference in, so that the code can be “normalized” as soon as that issue is fixed

@Blaisorblade Blaisorblade and 1 other commented on an outdated diff Sep 7, 2012
src/library/scala/concurrent/util/Duration.scala
- def *(factor: Double): Duration = this
- def /(factor: Double): Duration = this
+ def *(factor: Double): Duration =
+ if (factor == 0d || factor.isNaN) Undefined
+ else if (factor < 0d) -this
+ else this
+ def /(factor: Double): Duration =
+ if (factor == 0d || factor.isNaN || factor.isInfinite) Undefined
@Blaisorblade
Blaisorblade Sep 7, 2012 Contributor

I'd suggest Inf / 0 => Inf instead of Inf/0 => Undefined, consistently with the behavior of /(other: Duration), and being careful with the sign. I'd try to reuse the tricky code for deciding the sign, that is, (if ((this > Zero) ^ (other >= Zero)) -1 else 1).
UPDATE: On second thought, assuming -0d < 0d, it's enough to just drop the first case, factor == 0d ||.

@rkuhn
rkuhn Sep 7, 2012 Contributor

good point, you found a missing test case; your UPDATE is not enough, though, due to the -0d sickness it needs to use compare instead of <.

BTW: is Double (and Float?) the only example of a violation of the equals/compareTo contract on the JVM which actually ships as part of the platform?

@Blaisorblade
Blaisorblade Sep 8, 2012 Contributor

Ouch, thanks for the attention. It's good I made my assumption explicit, and bad I couldn't check myself. I agree it's odd that Double (and Float, yes) give such results, although clearly documented, but AFAIK the equals contract doesn't require consistent results before and after boxing. To me, it's mostly odd that -0d == 0d in Java. I'm confused about Scala, where that same expression can ultimately invoke (depending on boxing) -0d.equals(0d).

@rkuhn rkuhn fix some one more issue in Duration
- Inf / Zero == Inf
- add some more missing test cases
- clarify magic constant
- move exception descriptions into proper @throws docs
f41d3ad
@Blaisorblade Blaisorblade and 1 other commented on an outdated diff Sep 8, 2012
src/library/scala/concurrent/util/Duration.scala
class FiniteDuration(val length: Long, val unit: TimeUnit) extends Duration {
import Duration._
+ require(unit match {
+ /*
+ * sorted so that the first cases should be most-used ones, because enum
+ * is checked one after the other.
+ */
+ case NANOSECONDS length != Long.MinValue
+ case MICROSECONDS length <= 9223372036854775L && length >= -9223372036854775L
+ case MILLISECONDS length <= 9223372036854L && length >= -9223372036854L
+ case SECONDS length <= 9223372036L && length >= -9223372036L
+ case MINUTES length <= 153722867L && length >= -153722867L
+ case HOURS length <= 2562047L && length >= -2562047L
+ case DAYS length <= 106751L && length >= -106751L
@Blaisorblade
Blaisorblade Sep 8, 2012 Contributor

Could these magic constants be made non-magic as well?

@rkuhn
rkuhn Sep 8, 2012 Contributor

Given that the expressed limit is clearly stated in the class’ scaladoc comment, I don’t think it is worth calculating them at some other place and giving them names (which would also slow down the creation of every single Duration instance). And they wouldn’t really get any less magic by moving them into the companion to give them names, because then you’d have to search for their definition instead seeing a value which many programmers will intuitively associate with the right thing (granted, I also don’t know the digits by heart for all of them, but they are easily verified).

So, long story short, the best I can think of is adding comments saying how they were calculated.

@Blaisorblade Blaisorblade commented on the diff Sep 8, 2012
src/library/scala/concurrent/util/Duration.scala
def hasTimeLeft(): Boolean = !isOverdue()
+ /**
+ * Determine whether the deadline lies in the past at the point where this method is called.
+ *
+ * '''''Note that on some systems this operation is costly because it entails a system call.'''''
+ * Check `System.nanoTime` for your platform.
+ */
def isOverdue(): Boolean = (time.toNanos - System.nanoTime()) < 0
@Blaisorblade
Blaisorblade Sep 8, 2012 Contributor

Why isn't this just timeLeft < Duration.Zero, which is the specification? Writing it this way introduces some potential for bugs, now or in the future.
The only difference I see is a few object allocations less. Are there platforms where nanoTime is fast enough for this to be relevant (making this a non-system-call has been a goal of optimizations in Windows and Linux, I don't remember how successful)?
Of course the result should be the same, but given the possibility of overflow errors being present or introduced by future changes, there should be (outside the ScalaDoc) a comment which explains why this is equivalent, together with (stress?)-tests checking it. After a quick look, I'd propose:
"This is equivalent to timeLeft < Duration.Zero. Since toNanos is guaranteed to never overflow, and Deadline.time is guaranteed to be a positive Duration, there is no risk of overflow and the results are equivalent."
(Note: I didn't check whether there's any overflow risk for timeLeft < Duration.Zero, but in the end that code must work since it just uses the public interface as intended).

@viktorklang
viktorklang Sep 8, 2012 Contributor

Because timeLeft allocates a new object.

@Blaisorblade
Blaisorblade Sep 12, 2012 Contributor

Still, the invariant should be documented and tested. And on second thought, there's a bug already:

scala> Duration.Inf.fromNow
res0: scala.concurrent.util.Deadline = Deadline(Duration.Inf)

scala> Duration.Inf.fromNow.isOverdue
java.lang.IllegalArgumentException: toNanos not allowed on infinite Durations
    at scala.concurrent.util.Duration$Infinite$class.toNanos(Duration.scala:171)
    at scala.concurrent.util.Duration$$anon$2.toNanos(Duration.scala:186)
    at scala.concurrent.util.Deadline.isOverdue(Duration.scala:22)
(...)
@rkuhn
rkuhn Sep 12, 2012 Contributor

you are right, will fix by requiring FiniteDuration (nothing else makes sense)

@daggerrz daggerrz and 3 others commented on an outdated diff Sep 8, 2012
src/library/scala/concurrent/util/Duration.scala
+ private val µs_per_ns = 1000L
@daggerrz
daggerrz Sep 8, 2012

Making these private[this] would improve the performance of fromNanos. This is s.c.u, after all. :)

@paulp
paulp Sep 8, 2012 Contributor

Actually what will really improve it is making them final, which will cause the constants to be inlined directly, at which point the access level doesn't matter since the field will never be accessed.

@viktorklang
viktorklang Sep 8, 2012 Contributor

What's the reason final isn't implied when private?

@paulp
paulp Sep 8, 2012 Contributor

Consider a file like this:

object Foo {
  private val x = 5
  final val y = 10

  def f1 = () => x
  def f2 = () => y
}

f1 returns a closure which returns the value of x. f2 returns a closure which literally returns "10". If you recompile this with different values, the closure which returns 10 will still return 10. The one which accesses x will return the new value of x.

@rkuhn
rkuhn Sep 9, 2012 Contributor

thanks for noticing, making them private final val has exactly the right effect (i.e. inline the constants and hide them from Java).

@rkuhn rkuhn fix two minor issues in Duration
- make constants for fromNanos(Long) actually inlined constants
- clarify origin of require() check constants in FiniteDuration
ccf734a
@rkuhn
Contributor
rkuhn commented Sep 9, 2012

PLS REBUILD ALL

@rkuhn
Contributor
rkuhn commented Sep 9, 2012

@jsuereth This does not look like my code was even checked out before it failed?

@jsuereth
Member
jsuereth commented Sep 9, 2012

PLS REBUILD ALL

@viktorklang
Contributor

@paulp Thanks for the explanation Paul! Isn't that only an issue when you do hot code reloading though?

@paulp
Contributor
paulp commented Sep 9, 2012

It's an issue anytime there is separate compilation. If someone compiles against library version 1.0 and you ship 1.1, code built against 1.0 will use the final constant type vals from 1.0, not those from 1.1. This may be desirable or undesirable in any given case, but the point is that it's not the same.

@viktorklang
Contributor

@paulp In the example of the method returning a function returning the constant, that would only be true if the call to the method was replaced with the closure, or?

class Foo {
  private final val x = 3
  private val y = 5
  def fX = () => x
  def fY = () => y
}

val f = new Foo
f.fX.apply // call to fX, replacing Foo.class would make this return what the new Foo.class.fX returns?
f.fY.apply
@paulp
Contributor
paulp commented Sep 9, 2012

Maybe this will clarify things:

// a.scala
class A {
  private val x0 = 1823905
  def x1 = x0
  final val x2 = 1572398
}
// alt-a.scala
class A {
  private val x0 = 0
  def x1 = x0
  final val x2 = 0
}
// b.scala
class B {
  val a = new A
  val y1 = a.x1
  val y2 = a.x2
  override def toString = "(" + y1 + ", " + y2 + ")"
}

object Test {
  def main(args: Array[String]): Unit = {
    println(new B)
  }
}

// demo.sh
#!/bin/sh
#

scalac a.scala b.scala
scala Test
scalac alt-a.scala
scala Test

And:

% ./demo.sh 
(1823905, 1572398)
(0, 1572398)
@viktorklang
Contributor

@paulp Yes, that I fully understand. However, the "private final val"-case works as expected (make x0 private final).

@rkuhn
Contributor
rkuhn commented Sep 11, 2012

@viktorklang @jsuereth would you please review after my latest changes? Thanks.

@jsuereth jsuereth commented on the diff Sep 11, 2012
src/library/scala/concurrent/util/Duration.scala
+ /**
+ * Construct a finite duration from the given length and time unit. The unit given is retained
+ * throughout calculations as long as possible, so that it can be retrieved later.
+ */
+ def apply(length: Long, unit: TimeUnit): FiniteDuration = new FiniteDuration(length, unit)
+ /**
+ * Construct a finite duration from the given length and time unit, where the latter is
+ * looked up in a list of string representation. Valid choices are:
+ *
+ * `d, day, h, hour, min, minute, s, sec, second, ms, milli, millisecond, µs, micro, microsecond, ns, nano, nanosecond`
+ * and their pluralized forms (for every but the first mentioned form of each unit, i.e. no "ds", but "days").
+ */
+ def apply(length: Long, unit: String): FiniteDuration = new FiniteDuration(length, Duration.timeUnit(unit))
+
+ // Double stores 52 bits mantissa, but there is an implied '1' in front, making the limit 2^53
+ final val maxPreciseDouble = 9007199254740992d
@jsuereth
jsuereth Sep 11, 2012 Member

Type ascription police!

Since this is a public api, let's see:

final val maxPreciseDouble: Double = 9007199254740992d

Just in case someone ever drops the 'd' from the end in a crazy fit of refactoring.

@rkuhn
rkuhn Sep 11, 2012 Contributor

nope, this needs to be a constant, and unfortunately constants are not allowed to use the police or any armed forces :-(

@paulp
paulp Sep 11, 2012 Contributor

Yes, I realize I just said "always specify the return type in public api" but there was an implied exception here, because there is no choice. Anyway, if someone dropped the d it wouldn't even compile.

@jsuereth jsuereth commented on the diff Sep 11, 2012
src/library/scala/concurrent/util/Duration.scala
*/
val Inf: Infinite = new Infinite {
override def toString = "Duration.Inf"
- def compare(other: Duration) = if (other eq this) 0 else 1
+ def compare(other: Duration) = other match {
+ case x if x eq Undefined => -1
@jsuereth
jsuereth Sep 11, 2012 Member

case Undefined => -1 ??

@rkuhn
rkuhn Sep 11, 2012 Contributor

no, Undefined != Undefined (because it is supposed to behave like Double.NaN)

@jsuereth jsuereth and 1 other commented on an outdated diff Sep 11, 2012
src/library/scala/concurrent/util/Duration.scala
*/
val Inf: Infinite = new Infinite {
override def toString = "Duration.Inf"
- def compare(other: Duration) = if (other eq this) 0 else 1
+ def compare(other: Duration) = other match {
+ case x if x eq Undefined => -1
+ case x if x eq this => 0
@jsuereth
jsuereth Sep 11, 2012 Member

Note: In Fun Hilarity, you can probably safely do:

case `Inf` => 0
@rkuhn
rkuhn Sep 11, 2012 Contributor

actually it turns out that my version produces a lot less byte code, since your proposal will include code handling the case Inf == null (as nonsensical as that may seem)

@jsuereth jsuereth commented on the diff Sep 11, 2012
src/library/scala/concurrent/util/Duration.scala
}
/**
- * Infinite duration: greater than any other and not equal to any other,
- * including itself.
+ * Infinite duration: greater than any other (apart from Undefined) and not equal to any other
+ * but itself. This value closely corresponds to Double.PositiveInfinity,
+ * matching its semantics in arithmetic operations.
*/
val Inf: Infinite = new Infinite {
@jsuereth
jsuereth Sep 11, 2012 Member

No final?

@rkuhn
rkuhn Sep 11, 2012 Contributor

I’m not aware of any effect this would have; am I wrong?

@jsuereth jsuereth and 2 others commented on an outdated diff Sep 11, 2012
src/library/scala/concurrent/util/Duration.scala
class FiniteDuration(val length: Long, val unit: TimeUnit) extends Duration {
import Duration._
+ require(unit match {
+ /*
+ * enforce the 2^63-1 ns limit, must be pos/neg symmetrical because of unary_-
+ */
+ case NANOSECONDS length != Long.MinValue // max_ns = Long.MaxValue
+ case MICROSECONDS length <= 9223372036854775L && length >= -9223372036854775L // max_µs = max_ns / 1000
@jsuereth
jsuereth Sep 11, 2012 Member

Holy magic numbers, batman! Is there no object we can hide these on as final val?

@rkuhn
rkuhn Sep 11, 2012 Contributor

wouldn’t help, as discussed earlier: they will stay the same numbers, meaning a comment is the best that can be done, and then I’d prefer to have them in-line so people don’t need to go chasing them

@jsuereth
jsuereth Sep 11, 2012 Member

Ah, I missed that. Sounds ok then..., maybe a // about "Don't touch these unless you are (a) God (b) Roland Kuhn (c) have 3 or more reviewers"

@paulp
paulp Sep 11, 2012 Contributor

I don't see how a comment is the best which can be done.

// a few lines up, it's not much of a chase
final val maxNanos   = Long.MaxValue
final val maxMicros  = maxNanos / 1000
final val maxMillis  = maxMicros / 1000
final val maxSeconds = maxMillis / 1000
final val maxMinutes = maxSeconds / 60
final val maxHours   = maxMinutes / 60
final val maxDays    = maxHours / 24

import FiniteDuration._

require(unit match {
    /*
     * enforce the 2^63-1 ns limit, must be pos/neg symmetrical because of unary_-
     */
    case NANOSECONDS  ⇒   -maxNanos <= length && length <= maxNanos
    case MICROSECONDS ⇒  -maxMicros <= length && length <= maxMicros
    case MILLISECONDS ⇒  -maxMillis <= length && length <= maxMillis
    case SECONDS      ⇒ -maxSeconds <= length && length <= maxSeconds
    case MINUTES      ⇒ -maxMinutes <= length && length <= maxMinutes
    case HOURS        ⇒   -maxHours <= length && length <= maxHours
    case DAYS         ⇒    -maxDays <= length && length <= maxDays
    case _ ⇒
      val v = DAYS.convert(length, unit)
      -maxDays <= v && v <= maxDays
  }, "Duration is limited to +-(2^63-1)ns (ca. 292 years)")

You don't have to hardcode long strings of digits.

@rkuhn
rkuhn Sep 12, 2012 Contributor

Indeed, it turns out that my worries about the constants turning public were unfounded, and the overhead of one lneg in each case surely gets lost in the noise. I’m running the tests again and will push a fix shortly.

@jsuereth jsuereth commented on an outdated diff Sep 11, 2012
src/library/scala/concurrent/util/Duration.scala
@@ -289,20 +650,56 @@ class FiniteDuration(val length: Long, val unit: TimeUnit) extends Duration {
case x: FiniteDuration => toNanos compare x.toNanos
case _ => -(other compare this)
}
+
+ // see https://www.securecoding.cert.org/confluence/display/java/NUM00-J.+Detect+or+prevent+integer+overflow
+ private def safeAdd(a: Long, b: Long): Long = {
+ if ((b > 0) && (a > Long.MaxValue - b) ||
@jsuereth
jsuereth Sep 11, 2012 Member
`<joke>` Can we put ZERO in a constant to avoid this magic number? `</joke>`
@jsuereth jsuereth commented on the diff Sep 11, 2012
test/files/jvm/duration-tck.scala
@@ -0,0 +1,191 @@
+/**
+ * Copyright (C) 2012 Typesafe Inc. <http://www.typesafe.com>
+ */
+
+import scala.concurrent.util._
+import duration._
+import scala.reflect._
+import java.util.concurrent.TimeUnit._
+
+object Test extends App {
+
+ implicit class Assert(val left: Any) extends AnyVal {
+ import Duration.Undefined
+ def mustBe(right: Any) = right match {
@jsuereth
jsuereth Sep 11, 2012 Member

Sneaking some scalatest in?

@rkuhn
rkuhn Sep 11, 2012 Contributor

well, the test was ported from Akka and I didn’t want to change it more than necessary

@jsuereth jsuereth and 2 others commented on an outdated diff Sep 11, 2012
test/files/jvm/duration-tck.scala
+import scala.reflect._
+import java.util.concurrent.TimeUnit._
+
+object Test extends App {
+
+ implicit class Assert(val left: Any) extends AnyVal {
+ import Duration.Undefined
+ def mustBe(right: Any) = right match {
+ case r: Double if r.isNaN => assert(left.asInstanceOf[Double].isNaN, s"$left was not NaN")
+ case r: Double if r == 0 && r.compareTo(0) == -1 => assert(left == 0 && left.asInstanceOf[Double].compareTo(0) == -1, s"$left was not -0.0")
+ case Undefined => assert(left.asInstanceOf[AnyRef] eq Undefined, s"$left was not Undefined")
+ case _ => assert(left == right, s"$left was not equal to $right")
+ }
+ }
+
+ def intercept[T <: Exception : ClassTag](code: => Unit) =
@jsuereth
jsuereth Sep 11, 2012 Member

This is a highly useful method. Note: You can define abstract traits in src/partest that you can extend in these tests. I'd love to see intercept there.

Also note: scala.util.control.Exception.catching should probably look like this a bit too.

@rkuhn
rkuhn Sep 11, 2012 Contributor

yes, I did that in #1279, but since these branches are not based upon each other I cannot use it from here.

@jsuereth
jsuereth Sep 11, 2012 Member

Gotcha. We'll have to do an intercept refactor later.

@Blaisorblade
Blaisorblade Sep 12, 2012 Contributor

Notice: I remember at least another use of intercept (and friends) in FutureTests, TryTests, etc.

@jsuereth
Member

These changes are a great improvement! I have a few minor comments, then LGTM.

@rkuhn rkuhn two more minor cleanups to Duration
- copy partest TestUtil.intercept change from PR 1279 branch
- add comment on non-obvious match cases
4a476e7
@rkuhn
Contributor
rkuhn commented Sep 12, 2012

PLS REBUILD ALL

@retronym

Just because this is the most reviewed PR in the history of the repo...

private[this] def bounded(max: Long) = -max  <= length && length <= max

// ...
      case NANOSECONDS   bounded(max_ns)
Owner

Making it private instead of private[this] results in the exact same byte code; I guess invokespecial is heavily optimized so that the gain in readability does not come with a cost, right? (I think we’re talking nuances here)

But nuances are interesting :)

Private methods are importable. So, val d = new Duration(...); import d._ would stick bounded in scope; it can't be accessed other than inside Duration or its companion, but it can cause an ambiguous identifier error. @paulp has a branch for this (of course), but we're stuck with the current behaviour for at least this release.

Importing all the methods from an instance of Duration isn't likely to happen often; this is a much bigger problems with objects (especially of the package variety.)

Oh, and as to performance, its a tiny, monomorphic method, that's perfect hotspot fodder.

Duplication is everywhere, once you open your eyes to it. Luckily, Scala gives a reasonable set of tools to eradicate it.

Owner

Thanks for the explanation, will fix.

Owner

with that I mean: apply to all other places where it does work (i.e. not in the case of the FiniteDuration constants, but that cannot be helped); pushed.

@retronym

This should be final.

(So should hundreds of other classes in the stdlib, we'll be able to fix that now with @deprecatedInheritance.)

Owner

ah, right, missed this one

@Blaisorblade Blaisorblade and 1 other commented on an outdated diff Sep 12, 2012
src/library/scala/concurrent/util/Duration.scala
+/**
+ * This class stores a deadline, as obtained via `Deadline.now` or the
+ * duration DSL:
+ *
+ * {{{
+ * import scala.concurrent.util.duration._
+ * 3.seconds.fromNow
+ * }}}
+ *
+ * Its main purpose is to manage repeated attempts to achieve something (like
+ * awaiting a condition) by offering the methods `hasTimeLeft` and `timeLeft`. All
+ * durations are measured according to `System.nanoTime` aka wall-time; this
+ * does not take into account changes to the system clock (such as leap
+ * seconds).
+ */
+case class Deadline private (time: Duration) extends Ordered[Deadline] {
@Blaisorblade
Blaisorblade Sep 12, 2012 Contributor

It seems that time is in fact required to be a FiniteDuration, because e.g. isOverdue fails if time is not a FiniteDuration; if so, this should be checked statically by making time's type FiniteDuration. Consequently, you'd have to move fromNow into FiniteDuration to make it typecheck.
If instead Inf.fromNow is supposed to be legal, then isOverdue is buggy.

@rkuhn
rkuhn Sep 12, 2012 Contributor

Thanks, I did exactly as you proposed (move fromNow into FiniteDuration) and added a negative test so that (x: Duration).fromNow does not compile.

@rkuhn
Contributor
rkuhn commented Sep 12, 2012

PLS REBUILD ALL

@rkuhn
Contributor
rkuhn commented Sep 12, 2012

@jsuereth since you merged #1279 yesterday, there will be a (logical) conflict now between this and master. Shall I merge master in here and fix it up?

@rkuhn
Contributor
rkuhn commented Sep 12, 2012

PLS REBUILD ALL

@jsuereth
Member

Thanks for the hard work and cleanup!

@jsuereth jsuereth merged commit ea24210 into scala:2.10.x Sep 12, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment