Fine by me, but I'm not an OpenJDK Reviewer Stephen
On Mon, 9 Mar 2020 at 03:05, <naoto.s...@oracle.com> wrote: > > Thanks, Stephen. > > Updated the webrev for those two suggestions: > > http://cr.openjdk.java.net/~naoto/8239836/webrev.04/ > > Naoto > > On 3/8/20 4:22 PM, Stephen Colebourne wrote: > > 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 > >>>> > >>>>
