I fear more changes are needed here.
1) The code is isFixedOffset() is indeed wrong, but the fix is
insufficient. The zone is fixed if baseStandardOffset=baseWallOffset
and all three other lists are empty. A fixed offset rule is the
equivalent of a ZoneOffset. See ZoneId.normalized() for the code that
assumes that.
2) The code in getOffset(Instant) is wrong, but so is the proposed
fix. The purpose of the if statement is to optimise the following few
lines which refer to savingsInstantTransitions and wallOffsets. The
code does have a bug as it should return the first wall offset.
Corrected code:
public ZoneOffset getOffset(Instant instant) {
if (savingsInstantTransitions.length == 0) {
return wallOffsets[0];
}
With the correct definition of isFixedOffset() it becomes apparent
that this if statement is in fact not related to the fixed offset.
Currently this test case fails (a zone in DST for all eternity):
ZoneRules rules = ZoneRules.of(
ZoneOffset.ofHours(1),
ZoneOffset.ofHours(2),
Collections.emptyList(),
Collections.emptyList(),
Collections.emptyList());
assertEquals(rules.getStandardOffset(Instant.EPOCH),
ZoneOffset.ofHours(1));
assertEquals(rules.getOffset(Instant.EPOCH), ZoneOffset.ofHours(2));
3) The code in getStandardOffset(Instant) also has an error as it
checks the wrong array. It should check standardTransitions as that is
what is used in the following few lines. Corrected code:
public ZoneOffset getStandardOffset(Instant instant) {
if (standardTransitions.length == 0) {
return standardOffsets[0];
}
4) The code in getOffsetInfo(LocalDateTime dt) has a similar fault.
Since it protects savingsLocalTransitions, it should return
wallOffsets[0].
5) The code in getDaylightSavings(Instant instant) has an optimising
if statement that should refer to isFixedOffset().
6) Also, in the test.
assertTrue(!rules.isFixedOffset());
should be
assertFalse(rules.isFixedOffset());
The class has worked so far as every normal case has
baseStandardOffset = baseWallOffset, even though it is conceptually
valid for this not to be true.
Stephen
On Thu, 27 Feb 2020 at 20:42, <naoto.s...@oracle.com> wrote:
>
> Hello,
>
> Please review the fix to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8239836
>
> The proposed changeset is located at:
>
> https://cr.openjdk.java.net/~naoto/8239836/webrev.00/
>
> It turned out that 'transitionList' is not necessarily a superset of
> 'standardOffsetTransitionList', as in some occasions where a standard
> offset change and a savings change happen at the same time (canceling
> each other), resulting in no wall time transition. This means that the
> "rules" in the sample code is a valid ZoneRules instance.
>
> However, there is an assumption in ZoneRules where no (wall time)
> transition means the fixed offset zone. With that assumption, the
> example rule is considered a fixed offset zone which is not correct. So
> the fix proposed here is not to throw an IAE but to recognize the rule
> as a valid, non-fixed offset ZoneRules instance.
>
> Naoto
>
>
