Durations in existing JDK APIs

[Martin Buchholz]

TimeUnit#toDuration is gone.

Here's a non-strawman attempt at TimeUnit#convert(Duration).
I was astonished how hard it was to get this really correct instead of
merely mostly correct.
For negative numbers, the long/int representation of Duration is sort-of a
"floored" division instead of java's default "truncating" division.

    /**
     * Converts the given time duration to this unit.
     *
     * <p>For any TimeUnit {@code unit},
     * {@code unit.convert(Duration.ofNanos(n))}
     * is equivalent to
     * {@code unit.convert(n, NANOSECONDS)}, and
     * {@code unit.convert(Duration.of(n, unit.toChronoUnit()))}
     * is equivalent to {@code n} (in the absence of overflow).
     *
     * @param duration the time duration
     * @return the converted duration in this unit,
     * or {@code Long.MIN_VALUE} if conversion would negatively overflow,
     * or {@code Long.MAX_VALUE} if it would positively overflow.
     * @throws NullPointerException if {@code duration} is null
     * @see Duration#of(long,TemporalUnit)
     * @since 11
     */
    public long convert(Duration duration) {
        final long secs = duration.getSeconds();
        final int nano = duration.getNano();
        final long s, candidate;
        if ((s = scale) == SECOND_SCALE)
            return (secs < 0 && nano > 0) ? secs + 1 : secs;
        if (s > SECOND_SCALE) {
            long div = secs / secRatio;
            if (secs < 0 && nano > 0 && div * secRatio == secs)
                div++;
            return div;
        }
        return (secs >= 0)
            ? ((secs > maxSecs
                || (candidate = secs * secRatio + nano / s) < 0)
               ? Long.MAX_VALUE : candidate)
            : ((secs < - maxSecs - 1
                || (candidate = secs * secRatio + (nano + s - 1) / s) > 0)
               ? Long.MIN_VALUE : candidate);
    }

But is it really correct?  Here are some tests as well:


    /**
     * convert(Duration) roundtrips with Duration.ofXXXX and
Duration.of(long, ChronoUnit)
     */
    public void testConvertDuration_roundtripDurationOf() {
        long n = ThreadLocalRandom.current().nextLong();

        assertEquals(n, NANOSECONDS.convert(Duration.ofNanos(n)));
        assertEquals(n, NANOSECONDS.convert(Duration.of(n,
ChronoUnit.NANOS)));
        assertEquals(n, MILLISECONDS.convert(Duration.ofMillis(n)));
        assertEquals(n, MILLISECONDS.convert(Duration.of(n,
ChronoUnit.MILLIS)));
        assertEquals(n, SECONDS.convert(Duration.ofSeconds(n)));
        assertEquals(n, SECONDS.convert(Duration.of(n,
ChronoUnit.SECONDS)));
        n /= 60;
        assertEquals(n, MINUTES.convert(Duration.ofMinutes(n)));
        assertEquals(n, MINUTES.convert(Duration.of(n,
ChronoUnit.MINUTES)));
        n /= 60;
        assertEquals(n, HOURS.convert(Duration.ofHours(n)));
        assertEquals(n, HOURS.convert(Duration.of(n, ChronoUnit.HOURS)));
        n /= 24;
        assertEquals(n, DAYS.convert(Duration.ofDays(n)));
        assertEquals(n, DAYS.convert(Duration.of(n, ChronoUnit.DAYS)));
    }

    /**
     * convert(Duration.ofNanos(n)) agrees with convert(n, NANOSECONDS)
     */
    public void testConvertDuration_roundtripDurationOfNanos() {
        // Test values near unit transitions and near overflow.
        LongStream.concat(
                Arrays.stream(TimeUnit.values()).mapToLong(u ->
u.toNanos(1)),
                LongStream.of(Long.MAX_VALUE, Long.MIN_VALUE))
            .flatMap(n -> LongStream.of(n, n + 1, n - 1))
            .flatMap(n -> LongStream.of(n, n + 1_000_000_000, n -
1_000_000_000))
            .flatMap(n -> LongStream.of(n, -n))
            // .peek(System.err::println)
            .forEach(n -> Arrays.stream(TimeUnit.values()).forEach(
                u -> assertEquals(u.convert(n, NANOSECONDS),
                                  u.convert(Duration.ofNanos(n)))));
    }

    /**
     * convert(Duration) doesn't misbehave near Long.MAX_VALUE and
Long.MIN_VALUE.
     */
    public void testConvertDuration_nearOverflow() {
        ChronoUnit NANOS = ChronoUnit.NANOS;
        Duration maxDuration = Duration.ofSeconds(Long.MAX_VALUE,
999_999_999);
        Duration minDuration = Duration.ofSeconds(Long.MIN_VALUE, 0);

        for (TimeUnit u : TimeUnit.values()) {
            ChronoUnit cu = u.toChronoUnit();
            long r;
            if (u.toNanos(1) > SECONDS.toNanos(1)) {
                r = u.toNanos(1) / SECONDS.toNanos(1);

                assertThrows(ArithmeticException.class,
                             () -> Duration.of(Long.MAX_VALUE, cu),
                             () -> Duration.of(Long.MIN_VALUE, cu));
            } else {
                r = 1;

                Duration max = Duration.of(Long.MAX_VALUE, cu);
                Duration min = Duration.of(Long.MIN_VALUE, cu);
                assertEquals(Long.MAX_VALUE, u.convert(max));
                assertEquals(Long.MAX_VALUE - 1, u.convert(max.minus(1,
NANOS)));
                assertEquals(Long.MAX_VALUE - 1, u.convert(max.minus(1,
cu)));
                assertEquals(Long.MIN_VALUE, u.convert(min));
                assertEquals(Long.MIN_VALUE + 1, u.convert(min.plus(1,
NANOS)));
                assertEquals(Long.MIN_VALUE + 1, u.convert(min.plus(1,
cu)));
                assertEquals(Long.MAX_VALUE, u.convert(max.plus(1, NANOS)));
                if (u != SECONDS) {
                    assertEquals(Long.MAX_VALUE, u.convert(max.plus(1,
cu)));
                    assertEquals(Long.MIN_VALUE, u.convert(min.minus(1,
NANOS)));
                    assertEquals(Long.MIN_VALUE, u.convert(min.minus(1,
cu)));
                }
            }

            assertEquals(Long.MAX_VALUE / r, u.convert(maxDuration));
            assertEquals(Long.MIN_VALUE / r, u.convert(minDuration));
        }
    }




On Thu, May 31, 2018 at 12:32 AM, Stephen Colebourne <scolebourne at joda.org>
wrote:

> I'm not convinced TimeUnit::toDuration(long amount) has enough value.
> We don't have a similar method on ChronoUnit
>
> Duration.of(amount, timeUnit.toChronoUnit()) seems sufficient. Maybe
> document this in the convert(Duration) method?
>
> Stephen
>
>
> On 31 May 2018 at 01:19, Martin Buchholz <martinrb at google.com> wrote:
> > v.0.2 has both conversion methods in TimeUnit.  The unexpected weirdness
> is
> > that convert(Duration) saturates while toDuration throws
> > ArithmeticException, but both seem author-culture-consistent.  Perhaps
> > TimeUnit#toDuration doesn't provide enough value in view of the existing
> > Duration.of and TimeUnit#toChronoUnit.  And most of the time you'd
> expect to
> > convert from Duration to long, just before calling a TimeUnit based
> method.
> >
> >     /**
> >      * Converts the given time duration to this unit.
> >      *
> >      * @param duration the time duration
> >      * @return the converted duration in this unit,
> >      * or {@code Long.MIN_VALUE} if conversion would negatively overflow,
> >      * or {@code Long.MAX_VALUE} if it would positively overflow.
> >      * @throws NullPointerException if {@code duration} is null
> >      */
> >     public long convert(Duration duration) {
> >         long s = convert(duration.getSeconds(), SECONDS);
> >         if (s == Long.MIN_VALUE) return s;
> >         long n = convert(duration.getNano(), NANOSECONDS);
> >         assert n >= 0 && n < 1_000_000_000;
> >         return (s + n < s) ? Long.MAX_VALUE : s + n;
> >     }
> >
> >     /**
> >      * Converts the given time duration in this unit to a Duration.
> >      *
> >      * @param duration the time duration
> >      * @return the time duration represented as a Duration
> >      * @throws ArithmeticException if the duration cannot be represented
> >      * as a Duration due to numeric overflow
> >      */
> >     public Duration toDuration(long duration) {
> >         return Duration.of(duration, toChronoUnit());
> >     }
> >
>