RFR: JDK-8079063 ZoneOffsetTransition constructor should ignore nanoseconds
Stephen Colebourne
scolebourne at joda.org
Wed May 6 15:06:04 UTC 2015
I am also happy with this change.
Stephen
On 6 May 2015 at 15:43, Roger Riggs <Roger.Riggs at 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/webrev.01/
>>
>> What do you think?
>>
>> Regards, Peter
>>
>
More information about the core-libs-dev
mailing list