RFR: JDK-8079063 ZoneOffsetTransition constructor should ignore nanoseconds
Hi, Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063. Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places: 1 - the public static factory method: /** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { ...this one already disallows transition parameters that have transition.getNano() != 0. 2 - Lines 498..500 of ZoneOffsetTransitionRule: LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter); ...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition: /** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); } ...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not. 3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method: LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); } ...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds. The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition: * @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected: http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w... What do you think? Regards, Peter
Hi Peter, Thanks for the analysis and followup. Yes, I think thesimple check as you propose is the desired clarification. I agree that the change should not affect any existing code outside the JDK and does not raise a compatibility issue. Roger On 5/4/2015 6:22 AM, Peter Levart wrote:
Hi,
Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places:
1 - the public static factory method:
/** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
...this one already disallows transition parameters that have transition.getNano() != 0.
2 - Lines 498..500 of ZoneOffsetTransitionRule:
LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter);
...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition:
/** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); }
...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not.
3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); }
...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds.
The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition:
* @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value
This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
What do you think?
Regards, Peter
I am also happy with this change. Stephen On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Peter,
Thanks for the analysis and followup.
Yes, I think thesimple check as you propose is the desired clarification. I agree that the change should not affect any existing code outside the JDK and does not raise a compatibility issue.
Roger
On 5/4/2015 6:22 AM, Peter Levart wrote:
Hi,
Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places:
1 - the public static factory method:
/** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
...this one already disallows transition parameters that have transition.getNano() != 0.
2 - Lines 498..500 of ZoneOffsetTransitionRule:
LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter);
...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition:
/** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); }
...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not.
3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); }
...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds.
The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition:
* @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value
This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
What do you think?
Regards, Peter
Cool! Do we need any CCC approval as this *is* a spec change isn't it? I haven't done such a thing yet, so please give me some pointers. I also intend to add a jtreg test that verifies this new behavior. Regards, Peter On 05/06/2015 05:06 PM, Stephen Colebourne wrote:
I am also happy with this change.
Stephen
On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Peter,
Thanks for the analysis and followup.
Yes, I think thesimple check as you propose is the desired clarification. I agree that the change should not affect any existing code outside the JDK and does not raise a compatibility issue.
Roger
On 5/4/2015 6:22 AM, Peter Levart wrote:
Hi,
Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places:
1 - the public static factory method:
/** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
...this one already disallows transition parameters that have transition.getNano() != 0.
2 - Lines 498..500 of ZoneOffsetTransitionRule:
LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter);
...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition:
/** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); }
...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not.
3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); }
...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds.
The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition:
* @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value
This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
What do you think?
Regards, Peter
Hi Peter, I'll run the CCC side, mostly it follows the similar rationale and structure as the jira entry. And the details are in your webrev. Thanks, Roger On 5/6/2015 11:43 AM, Peter Levart wrote:
Cool!
Do we need any CCC approval as this *is* a spec change isn't it?
I haven't done such a thing yet, so please give me some pointers. I also intend to add a jtreg test that verifies this new behavior.
Regards, Peter
On 05/06/2015 05:06 PM, Stephen Colebourne wrote:
I am also happy with this change.
Stephen
On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Peter,
Thanks for the analysis and followup.
Yes, I think thesimple check as you propose is the desired clarification. I agree that the change should not affect any existing code outside the JDK and does not raise a compatibility issue.
Roger
On 5/4/2015 6:22 AM, Peter Levart wrote:
Hi,
Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places:
1 - the public static factory method:
/** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
...this one already disallows transition parameters that have transition.getNano() != 0.
2 - Lines 498..500 of ZoneOffsetTransitionRule:
LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter);
...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition:
/** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); }
...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not.
3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); }
...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds.
The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition:
* @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value
This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
What do you think?
Regards, Peter
Hi Roger, Now that CCC has approved the change in spec., I have prepared the final fix: http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w... I added asserts to package-private constructors as a means to catch possible future mistakes as constructors are sometimes called internally among the package classes directly. At least the constructor for ZoneOffsetTransition is. The ZoneOffsetTransitionRule constructor is not and could be made private as an alternative. What do you say? Regards, Peter On 05/06/2015 05:56 PM, Roger Riggs wrote:
Hi Peter,
I'll run the CCC side, mostly it follows the similar rationale and structure as the jira entry. And the details are in your webrev.
Thanks, Roger
On 5/6/2015 11:43 AM, Peter Levart wrote:
Cool!
Do we need any CCC approval as this *is* a spec change isn't it?
I haven't done such a thing yet, so please give me some pointers. I also intend to add a jtreg test that verifies this new behavior.
Regards, Peter
On 05/06/2015 05:06 PM, Stephen Colebourne wrote:
I am also happy with this change.
Stephen
On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Peter,
Thanks for the analysis and followup.
Yes, I think thesimple check as you propose is the desired clarification. I agree that the change should not affect any existing code outside the JDK and does not raise a compatibility issue.
Roger
On 5/4/2015 6:22 AM, Peter Levart wrote:
Hi,
Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places:
1 - the public static factory method:
/** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
...this one already disallows transition parameters that have transition.getNano() != 0.
2 - Lines 498..500 of ZoneOffsetTransitionRule:
LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter);
...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition:
/** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); }
...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not.
3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); }
...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds.
The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition:
* @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value
This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
What do you think?
Regards, Peter
Hi Peter, Looks good; thanks for the extra checks. Roger On 6/3/2015 9:45 AM, Peter Levart wrote:
Hi Roger,
Now that CCC has approved the change in spec., I have prepared the final fix:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
I added asserts to package-private constructors as a means to catch possible future mistakes as constructors are sometimes called internally among the package classes directly. At least the constructor for ZoneOffsetTransition is. The ZoneOffsetTransitionRule constructor is not and could be made private as an alternative. What do you say?
Regards, Peter
On 05/06/2015 05:56 PM, Roger Riggs wrote:
Hi Peter,
I'll run the CCC side, mostly it follows the similar rationale and structure as the jira entry. And the details are in your webrev.
Thanks, Roger
On 5/6/2015 11:43 AM, Peter Levart wrote:
Cool!
Do we need any CCC approval as this *is* a spec change isn't it?
I haven't done such a thing yet, so please give me some pointers. I also intend to add a jtreg test that verifies this new behavior.
Regards, Peter
On 05/06/2015 05:06 PM, Stephen Colebourne wrote:
I am also happy with this change.
Stephen
On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs@oracle.com> wrote:
Hi Peter,
Thanks for the analysis and followup.
Yes, I think thesimple check as you propose is the desired clarification. I agree that the change should not affect any existing code outside the JDK and does not raise a compatibility issue.
Roger
On 5/4/2015 6:22 AM, Peter Levart wrote:
Hi,
Now that JDK-8074003 is in, I'd like to discuss how to tackle JDK-8079063.
Package-private ZoneOffsetTransition constructor that takes LocalDateTime transition is invoked from the following 4 places:
1 - the public static factory method:
/** * Obtains an instance defining a transition between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param transition the transition date-time at the transition, which never * actually occurs, expressed local to the before offset, not null * @param offsetBefore the offset before the transition, not null * @param offsetAfter the offset at and after the transition, not null * @return the transition, not null * @throws IllegalArgumentException if {@code offsetBefore} and {@code offsetAfter} * are equal, or {@code transition.getNano()} returns non-zero value */ public static ZoneOffsetTransition of(LocalDateTime transition, ZoneOffset offsetBefore, ZoneOffset offsetAfter) {
...this one already disallows transition parameters that have transition.getNano() != 0.
2 - Lines 498..500 of ZoneOffsetTransitionRule:
LocalDateTime localDT = LocalDateTime.of(date, time); LocalDateTime transition = timeDefinition.createDateTime(localDT, standardOffset, offsetBefore); return new ZoneOffsetTransition(transition, offsetBefore, offsetAfter);
...where 'time' is an instance field of ZoneOffsetTransitionRule. The ZoneOffsetTransitionRule public static factory method has the following definition:
/** * Obtains an instance defining the yearly rule to create transitions between two offsets. * <p> * Applications should normally obtain an instance from {@link ZoneRules}. * This factory is only intended for use when creating {@link ZoneRules}. * * @param month the month of the month-day of the first day of the cutover week, not null * @param dayOfMonthIndicator the day of the month-day of the cutover week, positive if the week is that * day or later, negative if the week is that day or earlier, counting from the last day of the month, * from -28 to 31 excluding 0 * @param dayOfWeek the required day-of-week, null if the month-day should not be changed * @param time the cutover time in the 'before' offset, not null * @param timeEndOfDay whether the time is midnight at the end of day * @param timeDefnition how to interpret the cutover * @param standardOffset the standard offset in force at the cutover, not null * @param offsetBefore the offset before the cutover, not null * @param offsetAfter the offset after the cutover, not null * @return the rule, not null * @throws IllegalArgumentException if the day of month indicator is invalid * @throws IllegalArgumentException if the end of day flag is true when the time is not midnight */ public static ZoneOffsetTransitionRule of( Month month, int dayOfMonthIndicator, DayOfWeek dayOfWeek, LocalTime time, boolean timeEndOfDay, TimeDefinition timeDefnition, ZoneOffset standardOffset, ZoneOffset offsetBefore, ZoneOffset offsetAfter) { Objects.requireNonNull(month, "month"); Objects.requireNonNull(time, "time"); Objects.requireNonNull(timeDefnition, "timeDefnition"); Objects.requireNonNull(standardOffset, "standardOffset"); Objects.requireNonNull(offsetBefore, "offsetBefore"); Objects.requireNonNull(offsetAfter, "offsetAfter"); if (dayOfMonthIndicator < -28 || dayOfMonthIndicator > 31 || dayOfMonthIndicator == 0) { throw new IllegalArgumentException("Day of month indicator must be between -28 and 31 inclusive excluding zero"); } if (timeEndOfDay && time.equals(LocalTime.MIDNIGHT) == false) { throw new IllegalArgumentException("Time must be midnight when end of day flag is true"); } return new ZoneOffsetTransitionRule(month, dayOfMonthIndicator, dayOfWeek, time, timeEndOfDay, timeDefnition, standardOffset, offsetBefore, offsetAfter); }
...which allows 'time' parameter (that becomes ZoneOffsetTransitionRule's 'time' field) with no restriction as to whether it can contain nanos != 0 or not.
3, 4 - Lines 668..678 of ZoneRules private getOffsetInfo method:
LocalDateTime dtBefore = savingsLocalTransitions[index]; LocalDateTime dtAfter = savingsLocalTransitions[index + 1]; ZoneOffset offsetBefore = wallOffsets[index / 2]; ZoneOffset offsetAfter = wallOffsets[index / 2 + 1]; if (offsetAfter.getTotalSeconds() > offsetBefore.getTotalSeconds()) { // gap return new ZoneOffsetTransition(dtBefore, offsetBefore, offsetAfter); } else { // overlap return new ZoneOffsetTransition(dtAfter, offsetBefore, offsetAfter); }
...where dtBefore/dtAfter "transition" parameters are taken from savingsLocalTransitions[] array that is filled-in in ZoneRules constructors from passed-in ZoneOffsetTransition objects. So here no nanos != 0 can sneak in if ZoneOffsetTransition invariant holds.
The only place where nanos can sneak-in therefore seems to be the public ZoneOffsetTransitionRule.of() factory method. The question is whether the spec. could be changed so that ZoneOffsetTransitionRule.of() factory method would add another @throws definition:
* @throws IllegalArgumentException if {@code time.getNano()} returns non-zero value
This, I think, would be consistent with ZoneOffsetTransition.of() factory method and I believe early enough in the live of the API so that no custom software would be affected:
http://cr.openjdk.java.net/~plevart/jdk9-dev/ZoneOffsetTransitionRule.time/w...
What do you think?
Regards, Peter
participants (3)
-
Peter Levart
-
Roger Riggs
-
Stephen Colebourne