RFR: JDK-8079063 ZoneOffsetTransition constructor should ignore nanoseconds
Roger Riggs
Roger.Riggs at Oracle.com
Wed May 6 15:56:26 UTC 2015
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 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