RFR: JDK-8079063: ZoneOffsetTransitionRule.of should throw IAE for non-zero nanoseconds
Roger Riggs
Roger.Riggs at Oracle.com
Wed Jun 3 16:52:57 UTC 2015
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/webrev.02/
>
>
> 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 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