standardOffsets[0] == wallOffsets[0] needs to use .equals() Unrelated, there is a Javadoc/spec error at line 578 of the "new" file. It should be getValidOffsets, not getOffset.
Apart from that, it looks good. thanks Stephen On Wed, 4 Mar 2020 at 19:06, <naoto.s...@oracle.com> wrote: > > Hi Stephen, > > Thank you for the detailed explanation and correcting the > implementation. I incorporated all the suggestions below, as well as > adding a new test for isFixedOffset() implementation (with some clean-up): > > https://cr.openjdk.java.net/~naoto/8239836/webrev.03/ > > Please take a look at it. > > Naoto > > On 3/3/20 3:30 PM, Stephen Colebourne wrote: > > 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 > >> > >>
