I am also happy with this change. Stephen
On 6 May 2015 at 15:43, Roger Riggs <[email protected]> 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/webrev.01/ >> >> What do you think? >> >> Regards, Peter >> >
