-
Notifications
You must be signed in to change notification settings - Fork 110
Implement Comparable time marks in a time source returned by Clock.asTimeSource() #271
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,17 +40,50 @@ public fun Clock.todayIn(timeZone: TimeZone): LocalDate = | |
* Returns a [TimeSource] that uses this [Clock] to mark a time instant and to find the amount of time elapsed since that mark. | ||
*/ | ||
@ExperimentalTime | ||
public fun Clock.asTimeSource(): TimeSource = object : TimeSource { | ||
override fun markNow(): TimeMark = InstantTimeMark(now(), this@asTimeSource) | ||
public fun Clock.asTimeSource(): TimeSource.WithComparableMarks = object : TimeSource.WithComparableMarks { | ||
override fun markNow(): ComparableTimeMark = InstantTimeMark(now(), this@asTimeSource) | ||
} | ||
|
||
@ExperimentalTime | ||
private class InstantTimeMark(private val instant: Instant, private val clock: Clock) : TimeMark { | ||
override fun elapsedNow(): Duration = clock.now() - instant | ||
private class InstantTimeMark(private val instant: Instant, private val clock: Clock) : ComparableTimeMark { | ||
override fun elapsedNow(): Duration = saturatingDiff(clock.now(), instant) | ||
|
||
override fun plus(duration: Duration): TimeMark = InstantTimeMark(instant + duration, clock) | ||
override fun plus(duration: Duration): ComparableTimeMark = InstantTimeMark(instant.saturatingAdd(duration), clock) | ||
override fun minus(duration: Duration): ComparableTimeMark = InstantTimeMark(instant.saturatingAdd(-duration), clock) | ||
|
||
override fun minus(duration: Duration): TimeMark = InstantTimeMark(instant - duration, clock) | ||
override fun minus(other: ComparableTimeMark): Duration { | ||
if (other !is InstantTimeMark || other.clock != this.clock) { | ||
throw IllegalArgumentException("Subtracting or comparing time marks from different time sources is not possible: $this and $other") | ||
} | ||
return saturatingDiff(this.instant, other.instant) | ||
} | ||
|
||
override fun equals(other: Any?): Boolean { | ||
return other is InstantTimeMark && this.clock == other.clock && this.instant == other.instant | ||
} | ||
|
||
override fun hashCode(): Int = instant.hashCode() | ||
|
||
override fun toString(): String = instant.toString() | ||
|
||
private fun Instant.isSaturated() = this == Instant.MAX || this == Instant.MIN | ||
private fun Instant.saturatingAdd(duration: Duration): Instant { | ||
if (isSaturated()) { | ||
if (duration.isInfinite() && duration.isPositive() != this.isDistantFuture) { | ||
throw IllegalArgumentException("Summing infinities of different signs") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But we already have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
No, the operation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't it internally inconsistent? This would make sense in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's consistent in time mark infinity model: all the distant events are the same infinitely distant event. It's not consistent with math infinity, but we're ok with that. See the Libraries DM at 2023-04-11. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, ok. This model is so unintuitive to me that I didn't even think about it. Well, since the behavior is already established and you implemented it precisely, I guess there's no need to discuss anything further, as my gripes are with the behavior, not the implementation. |
||
} | ||
return this | ||
} | ||
return this + duration | ||
} | ||
private fun saturatingDiff(instant1: Instant, instant2: Instant): Duration = when { | ||
instant1 == instant2 -> | ||
Duration.ZERO | ||
instant1.isSaturated() || instant2.isSaturated() -> | ||
(instant1 - instant2) * Double.POSITIVE_INFINITY | ||
else -> | ||
instant1 - instant2 | ||
} | ||
} | ||
|
||
@Deprecated("Use Clock.todayIn instead", ReplaceWith("this.todayIn(timeZone)"), DeprecationLevel.WARNING) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,86 @@ | ||
/* | ||
* Copyright 2019-2023 JetBrains s.r.o. and contributors. | ||
* Use of this source code is governed by the Apache 2.0 License that can be found in the LICENSE.txt file. | ||
*/ | ||
|
||
package kotlinx.datetime.test | ||
|
||
import kotlinx.datetime.* | ||
import kotlin.test.* | ||
import kotlin.time.* | ||
import kotlin.time.Duration.Companion.days | ||
import kotlin.time.Duration.Companion.nanoseconds | ||
|
||
@OptIn(ExperimentalTime::class) | ||
class ClockTimeSourceTest { | ||
@Test | ||
fun arithmetic() { | ||
val timeSource = Clock.System.asTimeSource() | ||
val mark0 = timeSource.markNow() | ||
|
||
val markPast = mark0 - 1.days | ||
val markFuture = mark0 + 1.days | ||
|
||
assertTrue(markPast < mark0) | ||
assertTrue(markFuture > mark0) | ||
assertEquals(mark0, markPast + 1.days) | ||
assertEquals(2.days, markFuture - markPast) | ||
} | ||
|
||
@Test | ||
fun elapsed() { | ||
val clock = object : Clock { | ||
var instant = Clock.System.now() | ||
override fun now(): Instant = instant | ||
} | ||
val timeSource = clock.asTimeSource() | ||
val mark = timeSource.markNow() | ||
assertEquals(Duration.ZERO, mark.elapsedNow()) | ||
|
||
clock.instant += 1.days | ||
assertEquals(1.days, mark.elapsedNow()) | ||
|
||
clock.instant -= 2.days | ||
assertEquals(-1.days, mark.elapsedNow()) | ||
|
||
clock.instant = Instant.MAX | ||
assertEquals(Duration.INFINITE, mark.elapsedNow()) | ||
} | ||
|
||
@Test | ||
fun differentSources() { | ||
val mark1 = Clock.System.asTimeSource().markNow() | ||
val mark2 = object : Clock { | ||
override fun now(): Instant = Instant.DISTANT_FUTURE | ||
}.asTimeSource().markNow() | ||
assertNotEquals(mark1, mark2) | ||
assertFailsWith<IllegalArgumentException> { mark1 - mark2 } | ||
assertFailsWith<IllegalArgumentException> { mark1 compareTo mark2 } | ||
} | ||
|
||
@Test | ||
fun saturation() { | ||
val mark0 = Clock.System.asTimeSource().markNow() | ||
|
||
val markFuture = mark0 + Duration.INFINITE | ||
val markPast = mark0 - Duration.INFINITE | ||
|
||
for (delta in listOf(Duration.ZERO, 1.nanoseconds, 1.days)) { | ||
assertEquals(markFuture, markFuture - delta) | ||
assertEquals(markFuture, markFuture + delta) | ||
|
||
assertEquals(markPast, markPast - delta) | ||
assertEquals(markPast, markPast + delta) | ||
} | ||
val infinitePairs = listOf(markFuture to markPast, markFuture to mark0, mark0 to markPast) | ||
for ((later, earlier) in infinitePairs) { | ||
assertEquals(Duration.INFINITE, later - earlier) | ||
assertEquals(-Duration.INFINITE, earlier - later) | ||
} | ||
assertEquals(Duration.ZERO, markFuture - markFuture) | ||
assertEquals(Duration.ZERO, markPast - markPast) | ||
Comment on lines
+80
to
+81
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This highlights that we have a problem with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are not instants, but time marks, they have different saturation rules. Some are infinitely distant, but we defined the difference between them as zero. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we can't consider Looks like the difference is just the following:
So, we have two models of infinity:
Personally, after thinking about it, I like the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, me neither. What I see value in, is making sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's expected that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
By the same logic, wouldn't we also expect the following to be true? assertEquals(-Duration.INFINITE, instant - (instant + Duration.INFINITE))
Sure, why not consider There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this is out of scope of this PR |
||
|
||
assertFailsWith<IllegalArgumentException> { markFuture - Duration.INFINITE } | ||
assertFailsWith<IllegalArgumentException> { markPast + Duration.INFINITE } | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wary of time marks with different clocks looking the same but not being equal. I don't think anyone will want to parse these, so I don't see any downsides to including the
clock
in the output as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, makes sense