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
