Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
On Fri, Jan 29, 2016 at 9:37 AM, Stephen Colebourne wrote: > In the ideal fix, all methods that take the combination (long, > TimeUnit) would be supplemented by an override that takes (Duration). > eg. Future.get(long, TimeUnit) has an additional get(Duration). > However, this is a lot of work, might be unpopular and would be slower > for j.u.concurrent use. Of course, having (long, TimeUnit) argument pairs is a performance hack, but there's not much demand for an improved API, and I still hope that someday we'll have proper value types in Java, at which time adding support for value type durations will be more attractive. So let's do nothing else for now, unless there's a more urgent compelling reason?
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi Martin, On 1/29/2016 2:28 PM, Martin Buchholz wrote: On Fri, Jan 29, 2016 at 11:17 AM, Roger Riggs wrote: Hi Martin, Where did IllegalMonitorStateException creep in from? Oops. My Emacs keeps suggesting the wrong completion. Fixed. Should I update the CCC I started or will you start fresh? If you could update the CCC, that would be awesome! The only substantive change was removing a @throws IAE and adding @throws NPE. ok Roger
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
On Fri, Jan 29, 2016 at 11:17 AM, Roger Riggs wrote: > Hi Martin, > > Where did IllegalMonitorStateException creep in from? Oops. My Emacs keeps suggesting the wrong completion. Fixed. > Should I update the CCC I started or will you start fresh? If you could update the CCC, that would be awesome! The only substantive change was removing a @throws IAE and adding @throws NPE.
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi Martin,
Where did IllegalMonitorStateException creep in from?
Should I update the CCC I started or will you start fresh?
Otherwise, ok.
Roger
On 1/29/2016 2:12 PM, Martin Buchholz wrote:
Here is a proposed alternative patch (against jsr166 CVS).
Code is reworded and reformatted.
Tests are junit-ified.
Round-trip tests are added.
toChronoUnit no longer throws IAE, because it cannot (and we commit to
having ChronoUnit be a superset of TimeUnit, in perpetuity;
toChronoUnit will forever be an injective function).
Index: src/main/java/util/concurrent/TimeUnit.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/TimeUnit.java,v
retrieving revision 1.47
diff -u -r1.47 TimeUnit.java
--- src/main/java/util/concurrent/TimeUnit.java 20 Sep 2015 17:03:23 - 1.47
+++ src/main/java/util/concurrent/TimeUnit.java 29 Jan 2016 19:05:01 -
@@ -6,6 +6,9 @@
package java.util.concurrent;
+import java.time.temporal.ChronoUnit;
+import java.util.Objects;
+
/**
* A {@code TimeUnit} represents time durations at a given unit of
* granularity and provides utility methods to convert across units,
@@ -361,4 +364,48 @@
}
}
+/**
+ * Converts this {@code TimeUnit} to the equivalent {@code ChronoUnit}.
+ *
+ * @return the converted equivalent ChronoUnit
+ * @since 9
+ */
+public ChronoUnit toChronoUnit() {
+switch (this) {
+case NANOSECONDS: return ChronoUnit.NANOS;
+case MICROSECONDS: return ChronoUnit.MICROS;
+case MILLISECONDS: return ChronoUnit.MILLIS;
+case SECONDS: return ChronoUnit.SECONDS;
+case MINUTES: return ChronoUnit.MINUTES;
+case HOURS:return ChronoUnit.HOURS;
+case DAYS: return ChronoUnit.DAYS;
+default: throw new AssertionError();
+}
+}
+
+/**
+ * Converts a {@code ChronoUnit} to the equivalent {@code TimeUnit}.
+ *
+ * @param chronoUnit the ChronoUnit to convert
+ * @return the converted equivalent TimeUnit
+ * @throws IllegalArgumentException if {@code chronoUnit} has no
+ * equivalent TimeUnit
+ * @throws NullPointerException if {@code chronoUnit} is null
+ * @since 9
+ */
+public static TimeUnit of(ChronoUnit chronoUnit) {
+switch (Objects.requireNonNull(chronoUnit, "chronoUnit")) {
+case NANOS: return TimeUnit.NANOSECONDS;
+case MICROS: return TimeUnit.MICROSECONDS;
+case MILLIS: return TimeUnit.MILLISECONDS;
+case SECONDS: return TimeUnit.SECONDS;
+case MINUTES: return TimeUnit.MINUTES;
+case HOURS: return TimeUnit.HOURS;
+case DAYS:return TimeUnit.DAYS;
+default:
+throw new IllegalArgumentException(
+"No TimeUnit equivalent for " + chronoUnit);
+}
+}
+
}
Index: src/test/tck/TimeUnitTest.java
===
RCS file: /export/home/jsr166/jsr166/jsr166/src/test/tck/TimeUnitTest.java,v
retrieving revision 1.25
diff -u -r1.25 TimeUnitTest.java
--- src/test/tck/TimeUnitTest.java 25 Apr 2015 04:55:31 - 1.25
+++ src/test/tck/TimeUnitTest.java 29 Jan 2016 19:05:02 -
@@ -14,6 +14,7 @@
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
+import java.time.temporal.ChronoUnit;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
@@ -433,4 +434,49 @@
assertSame(x, serialClone(x));
}
+/**
+ * tests for toChronoUnit.
+ */
+public void testToChronoUnit() throws Exception {
+assertSame(ChronoUnit.NANOS, NANOSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MICROS, MICROSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MILLIS, MILLISECONDS.toChronoUnit());
+assertSame(ChronoUnit.SECONDS, SECONDS.toChronoUnit());
+assertSame(ChronoUnit.MINUTES, MINUTES.toChronoUnit());
+assertSame(ChronoUnit.HOURS, HOURS.toChronoUnit());
+assertSame(ChronoUnit.DAYS,DAYS.toChronoUnit());
+
+// Every TimeUnit has a defined ChronoUnit equivalent
+for (TimeUnit x : TimeUnit.values())
+assertSame(x, TimeUnit.of(x.toChronoUnit()));
+}
+
+/**
+ * tests for TimeUnit.of(ChronoUnit).
+ */
+public void testTimeUnitOf() throws Exception {
+assertSame(NANOSECONDS, TimeUnit.of(ChronoUnit.NANOS));
+assertSame(MICROSECONDS, TimeUnit.of(ChronoUnit.MICROS));
+assertSame(MILLISECONDS, TimeUnit.of(ChronoUnit.MILLIS));
+assertSame(SECONDS, TimeUnit.of(ChronoUnit.SECONDS));
+assertSame(MINUTES, TimeUnit.of(ChronoUnit.MINUTES));
+assertSame(HOURS,TimeUnit.of(ChronoUnit.HOURS));
+assertSame(DAYS, TimeUnit.of(ChronoUnit.DAYS));
+
+assertThrows(
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Here is a proposed alternative patch (against jsr166 CVS).
Code is reworded and reformatted.
Tests are junit-ified.
Round-trip tests are added.
toChronoUnit no longer throws IAE, because it cannot (and we commit to
having ChronoUnit be a superset of TimeUnit, in perpetuity;
toChronoUnit will forever be an injective function).
Index: src/main/java/util/concurrent/TimeUnit.java
===
RCS file:
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/TimeUnit.java,v
retrieving revision 1.47
diff -u -r1.47 TimeUnit.java
--- src/main/java/util/concurrent/TimeUnit.java 20 Sep 2015 17:03:23 - 1.47
+++ src/main/java/util/concurrent/TimeUnit.java 29 Jan 2016 19:05:01 -
@@ -6,6 +6,9 @@
package java.util.concurrent;
+import java.time.temporal.ChronoUnit;
+import java.util.Objects;
+
/**
* A {@code TimeUnit} represents time durations at a given unit of
* granularity and provides utility methods to convert across units,
@@ -361,4 +364,48 @@
}
}
+/**
+ * Converts this {@code TimeUnit} to the equivalent {@code ChronoUnit}.
+ *
+ * @return the converted equivalent ChronoUnit
+ * @since 9
+ */
+public ChronoUnit toChronoUnit() {
+switch (this) {
+case NANOSECONDS: return ChronoUnit.NANOS;
+case MICROSECONDS: return ChronoUnit.MICROS;
+case MILLISECONDS: return ChronoUnit.MILLIS;
+case SECONDS: return ChronoUnit.SECONDS;
+case MINUTES: return ChronoUnit.MINUTES;
+case HOURS:return ChronoUnit.HOURS;
+case DAYS: return ChronoUnit.DAYS;
+default: throw new AssertionError();
+}
+}
+
+/**
+ * Converts a {@code ChronoUnit} to the equivalent {@code TimeUnit}.
+ *
+ * @param chronoUnit the ChronoUnit to convert
+ * @return the converted equivalent TimeUnit
+ * @throws IllegalArgumentException if {@code chronoUnit} has no
+ * equivalent TimeUnit
+ * @throws NullPointerException if {@code chronoUnit} is null
+ * @since 9
+ */
+public static TimeUnit of(ChronoUnit chronoUnit) {
+switch (Objects.requireNonNull(chronoUnit, "chronoUnit")) {
+case NANOS: return TimeUnit.NANOSECONDS;
+case MICROS: return TimeUnit.MICROSECONDS;
+case MILLIS: return TimeUnit.MILLISECONDS;
+case SECONDS: return TimeUnit.SECONDS;
+case MINUTES: return TimeUnit.MINUTES;
+case HOURS: return TimeUnit.HOURS;
+case DAYS:return TimeUnit.DAYS;
+default:
+throw new IllegalArgumentException(
+"No TimeUnit equivalent for " + chronoUnit);
+}
+}
+
}
Index: src/test/tck/TimeUnitTest.java
===
RCS file: /export/home/jsr166/jsr166/jsr166/src/test/tck/TimeUnitTest.java,v
retrieving revision 1.25
diff -u -r1.25 TimeUnitTest.java
--- src/test/tck/TimeUnitTest.java 25 Apr 2015 04:55:31 - 1.25
+++ src/test/tck/TimeUnitTest.java 29 Jan 2016 19:05:02 -
@@ -14,6 +14,7 @@
import static java.util.concurrent.TimeUnit.NANOSECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
+import java.time.temporal.ChronoUnit;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
@@ -433,4 +434,49 @@
assertSame(x, serialClone(x));
}
+/**
+ * tests for toChronoUnit.
+ */
+public void testToChronoUnit() throws Exception {
+assertSame(ChronoUnit.NANOS, NANOSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MICROS, MICROSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MILLIS, MILLISECONDS.toChronoUnit());
+assertSame(ChronoUnit.SECONDS, SECONDS.toChronoUnit());
+assertSame(ChronoUnit.MINUTES, MINUTES.toChronoUnit());
+assertSame(ChronoUnit.HOURS, HOURS.toChronoUnit());
+assertSame(ChronoUnit.DAYS,DAYS.toChronoUnit());
+
+// Every TimeUnit has a defined ChronoUnit equivalent
+for (TimeUnit x : TimeUnit.values())
+assertSame(x, TimeUnit.of(x.toChronoUnit()));
+}
+
+/**
+ * tests for TimeUnit.of(ChronoUnit).
+ */
+public void testTimeUnitOf() throws Exception {
+assertSame(NANOSECONDS, TimeUnit.of(ChronoUnit.NANOS));
+assertSame(MICROSECONDS, TimeUnit.of(ChronoUnit.MICROS));
+assertSame(MILLISECONDS, TimeUnit.of(ChronoUnit.MILLIS));
+assertSame(SECONDS, TimeUnit.of(ChronoUnit.SECONDS));
+assertSame(MINUTES, TimeUnit.of(ChronoUnit.MINUTES));
+assertSame(HOURS,TimeUnit.of(ChronoUnit.HOURS));
+assertSame(DAYS, TimeUnit.of(ChronoUnit.DAYS));
+
+assertThrows(NullPointerException.class,
+ () -> TimeUnit.of((ChronoUnit)null));
+
+// ChronoUnits either round trip to their TimeUnit
+// equivalents, or throw IllegalMonitorStateExceptio
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
In the ideal fix, all methods that take the combination (long, TimeUnit) would be supplemented by an override that takes (Duration). eg. Future.get(long, TimeUnit) has an additional get(Duration). However, this is a lot of work, might be unpopular and would be slower for j.u.concurrent use. The proposal is minimal, allowing the conversion to occur more easily. The Duration.of(long, TemporalUnit) could then be used to create a Duration. I'll note again that the alternative to this would be to make TimeUnit implement TemporalUnit. Whether that is less confusing is an open question. The specific question asked is whether there should be a TimeUnit::toDuration(long) method. I'm not overly fussed, as I think there are probably enough alternatives. Stephen On 29 January 2016 at 17:24, Martin Buchholz wrote: > I propose that we the jsr166 maintainers take over this change (sorry > for butting in!), pushing it into openjdk from jsr166 CVS. > The tests as they are written today won't work because > TimeUnit/Basic.java is not a testng test. > But I don't think we should fix that - instead, tests for these > methods should be added to our tck tests (I can do that). > Coincidentally, I am close to committing our tck tests to openjdk. > > As for the new API - the simple changes in the webrev are perfectly > fine, but I expected to see some additional conversions. TimeUnit is > for expressing elapsed time durations (even though j.u.c. doesn't have > a separate class for that), so I vaguely expect conversions to/from > Duration. But we won't do anything unless date/time experts > (Stephen?) think it's a good idea. > > > On Fri, Jan 29, 2016 at 8:46 AM, Martin Buchholz wrote: >> I missed that this was modifying jsr166 files - looking now... >> >> On Fri, Jan 29, 2016 at 7:12 AM, Chris Hegarty >> wrote: >>> On 29/01/16 14:52, Roger Riggs wrote: Hi Nadeesh, Looks fine, Thanks, Roger On 1/27/2016 11:34 AM, nadeesh tv wrote: > > Hi all, > > Thanks for the suggestions. > Please see the updated webrev > http://cr.openjdk.java.net/~ntv/8141452/webrev.01/ >>> >>> >>> +1 This looks fine. >>> >>> Martin, Doug, >>> >>> I assume you are ok to accept this small change in >>> java.util.concurrent.TimeUnit. >>> >>> -Chris. >>> >>> > Regards, > Nadeesh TV > > On 1/25/2016 10:24 PM, Roger Riggs wrote: >> >> Hi Stephen, Nadeesh, >> >> TimeUnit.toChronoUnit is a static method. It seems redundant to have >> to pass an instance to a static method of its type. >> cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); >> >> Instead of: >>TimeUnit tu = TimeUnit.SECONDS; >> ChronoUnit cu = tu.toChronoUnit(); >> >> >> Minor edits please: >> >> in @param and @return use the type name when referring to the type. >> For example, TimeUnit vs timeUnit (the parameter). >> >> in @throws, use the parameter name instead of "the unit"; >> For example, >> + * @throws IllegalArgumentException if timeUnit cannot be converted >> Thanks, Roger >> >> On 1/25/2016 11:06 AM, nadeesh tv wrote: >>> >>> Hi all, >>> >>> Please see the updated webrev >>> http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ >>> >>> -- >>> Thanks and Regards, >>> Nadeesh TV >>> >>> >>> On 1/25/2016 9:01 PM, Stephen Colebourne wrote: Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: > Hi all, > > Please review a fix for conversion between Chronounit and Timeunit > > Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 > > webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ > > -- > Thanks and Regards, > Nadeesh TV > > >>> >> > > -- > Thanks and Regards, > Nadeesh TV > >>>
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
I propose that we the jsr166 maintainers take over this change (sorry for butting in!), pushing it into openjdk from jsr166 CVS. The tests as they are written today won't work because TimeUnit/Basic.java is not a testng test. But I don't think we should fix that - instead, tests for these methods should be added to our tck tests (I can do that). Coincidentally, I am close to committing our tck tests to openjdk. As for the new API - the simple changes in the webrev are perfectly fine, but I expected to see some additional conversions. TimeUnit is for expressing elapsed time durations (even though j.u.c. doesn't have a separate class for that), so I vaguely expect conversions to/from Duration. But we won't do anything unless date/time experts (Stephen?) think it's a good idea. On Fri, Jan 29, 2016 at 8:46 AM, Martin Buchholz wrote: > I missed that this was modifying jsr166 files - looking now... > > On Fri, Jan 29, 2016 at 7:12 AM, Chris Hegarty > wrote: >> On 29/01/16 14:52, Roger Riggs wrote: >>> >>> Hi Nadeesh, >>> >>> Looks fine, >>> >>> Thanks, Roger >>> >>> >>> On 1/27/2016 11:34 AM, nadeesh tv wrote: Hi all, Thanks for the suggestions. Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.01/ >> >> >> +1 This looks fine. >> >> Martin, Doug, >> >> I assume you are ok to accept this small change in >> java.util.concurrent.TimeUnit. >> >> -Chris. >> >> Regards, Nadeesh TV On 1/25/2016 10:24 PM, Roger Riggs wrote: > > Hi Stephen, Nadeesh, > > TimeUnit.toChronoUnit is a static method. It seems redundant to have > to pass an instance to a static method of its type. > cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); > > Instead of: >TimeUnit tu = TimeUnit.SECONDS; > ChronoUnit cu = tu.toChronoUnit(); > > > Minor edits please: > > in @param and @return use the type name when referring to the type. > For example, TimeUnit vs timeUnit (the parameter). > > in @throws, use the parameter name instead of "the unit"; > For example, > + * @throws IllegalArgumentException if timeUnit cannot be converted > Thanks, Roger > > On 1/25/2016 11:06 AM, nadeesh tv wrote: >> >> Hi all, >> >> Please see the updated webrev >> http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ >> >> -- >> Thanks and Regards, >> Nadeesh TV >> >> >> On 1/25/2016 9:01 PM, Stephen Colebourne wrote: >>> >>> Typo "TimeUnitequivalent" >>> Otherwise looks good. >>> thanks >>> Stephen >>> >>> >>> >>> On 25 January 2016 at 15:25, nadeesh tv wrote: >>> Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV >> > -- Thanks and Regards, Nadeesh TV >>> >>
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
I missed that this was modifying jsr166 files - looking now... On Fri, Jan 29, 2016 at 7:12 AM, Chris Hegarty wrote: > On 29/01/16 14:52, Roger Riggs wrote: >> >> Hi Nadeesh, >> >> Looks fine, >> >> Thanks, Roger >> >> >> On 1/27/2016 11:34 AM, nadeesh tv wrote: >>> >>> Hi all, >>> >>> Thanks for the suggestions. >>> Please see the updated webrev >>> http://cr.openjdk.java.net/~ntv/8141452/webrev.01/ > > > +1 This looks fine. > > Martin, Doug, > > I assume you are ok to accept this small change in > java.util.concurrent.TimeUnit. > > -Chris. > > >>> Regards, >>> Nadeesh TV >>> >>> On 1/25/2016 10:24 PM, Roger Riggs wrote: Hi Stephen, Nadeesh, TimeUnit.toChronoUnit is a static method. It seems redundant to have to pass an instance to a static method of its type. cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); Instead of: TimeUnit tu = TimeUnit.SECONDS; ChronoUnit cu = tu.toChronoUnit(); Minor edits please: in @param and @return use the type name when referring to the type. For example, TimeUnit vs timeUnit (the parameter). in @throws, use the parameter name instead of "the unit"; For example, + * @throws IllegalArgumentException if timeUnit cannot be converted Thanks, Roger On 1/25/2016 11:06 AM, nadeesh tv wrote: > > Hi all, > > Please see the updated webrev > http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ > > -- > Thanks and Regards, > Nadeesh TV > > > On 1/25/2016 9:01 PM, Stephen Colebourne wrote: >> >> Typo "TimeUnitequivalent" >> Otherwise looks good. >> thanks >> Stephen >> >> >> >> On 25 January 2016 at 15:25, nadeesh tv wrote: >> >>> Hi all, >>> >>> Please review a fix for conversion between Chronounit and Timeunit >>> >>> Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 >>> >>> webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ >>> >>> -- >>> Thanks and Regards, >>> Nadeesh TV >>> >>> > >>> >>> -- >>> Thanks and Regards, >>> Nadeesh TV >>> >> >
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
On 29/01/16 14:52, Roger Riggs wrote: Hi Nadeesh, Looks fine, Thanks, Roger On 1/27/2016 11:34 AM, nadeesh tv wrote: Hi all, Thanks for the suggestions. Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.01/ +1 This looks fine. Martin, Doug, I assume you are ok to accept this small change in java.util.concurrent.TimeUnit. -Chris. Regards, Nadeesh TV On 1/25/2016 10:24 PM, Roger Riggs wrote: Hi Stephen, Nadeesh, TimeUnit.toChronoUnit is a static method. It seems redundant to have to pass an instance to a static method of its type. cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); Instead of: TimeUnit tu = TimeUnit.SECONDS; ChronoUnit cu = tu.toChronoUnit(); Minor edits please: in @param and @return use the type name when referring to the type. For example, TimeUnit vs timeUnit (the parameter). in @throws, use the parameter name instead of "the unit"; For example, + * @throws IllegalArgumentException if timeUnit cannot be converted Thanks, Roger On 1/25/2016 11:06 AM, nadeesh tv wrote: Hi all, Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV On 1/25/2016 9:01 PM, Stephen Colebourne wrote: Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV -- Thanks and Regards, Nadeesh TV
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi Nadeesh, Looks fine, Thanks, Roger On 1/27/2016 11:34 AM, nadeesh tv wrote: Hi all, Thanks for the suggestions. Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.01/ Regards, Nadeesh TV On 1/25/2016 10:24 PM, Roger Riggs wrote: Hi Stephen, Nadeesh, TimeUnit.toChronoUnit is a static method. It seems redundant to have to pass an instance to a static method of its type. cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); Instead of: TimeUnit tu = TimeUnit.SECONDS; ChronoUnit cu = tu.toChronoUnit(); Minor edits please: in @param and @return use the type name when referring to the type. For example, TimeUnit vs timeUnit (the parameter). in @throws, use the parameter name instead of "the unit"; For example, + * @throws IllegalArgumentException if timeUnit cannot be converted Thanks, Roger On 1/25/2016 11:06 AM, nadeesh tv wrote: Hi all, Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV On 1/25/2016 9:01 PM, Stephen Colebourne wrote: Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV -- Thanks and Regards, Nadeesh TV
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi all, Thanks for the suggestions. Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.01/ Regards, Nadeesh TV On 1/25/2016 10:24 PM, Roger Riggs wrote: Hi Stephen, Nadeesh, TimeUnit.toChronoUnit is a static method. It seems redundant to have to pass an instance to a static method of its type. cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); Instead of: TimeUnit tu = TimeUnit.SECONDS; ChronoUnit cu = tu.toChronoUnit(); Minor edits please: in @param and @return use the type name when referring to the type. For example, TimeUnit vs timeUnit (the parameter). in @throws, use the parameter name instead of "the unit"; For example, + * @throws IllegalArgumentException if timeUnit cannot be converted Thanks, Roger On 1/25/2016 11:06 AM, nadeesh tv wrote: Hi all, Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV On 1/25/2016 9:01 PM, Stephen Colebourne wrote: Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV -- Thanks and Regards, Nadeesh TV
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi Stephen, Nadeesh, TimeUnit.toChronoUnit is a static method. It seems redundant to have to pass an instance to a static method of its type. cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS); Instead of: TimeUnit tu = TimeUnit.SECONDS; ChronoUnit cu = tu.toChronoUnit(); Minor edits please: in @param and @return use the type name when referring to the type. For example, TimeUnit vs timeUnit (the parameter). in @throws, use the parameter name instead of "the unit"; For example, + * @throws IllegalArgumentException if timeUnit cannot be converted Thanks, Roger On 1/25/2016 11:06 AM, nadeesh tv wrote: Hi all, Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV On 1/25/2016 9:01 PM, Stephen Colebourne wrote: Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi all, Please see the updated webrev http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV On 1/25/2016 9:01 PM, Stephen Colebourne wrote: Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV -- Thanks and Regards, Nadeesh TV
RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Hi all, Please review a fix for conversion between Chronounit and Timeunit Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ -- Thanks and Regards, Nadeesh TV
Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit
Typo "TimeUnitequivalent" Otherwise looks good. thanks Stephen On 25 January 2016 at 15:25, nadeesh tv wrote: > Hi all, > > Please review a fix for conversion between Chronounit and Timeunit > > Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452 > > webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/ > > -- > Thanks and Regards, > Nadeesh TV > >
