Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
+1
On 3/9/20 12:05 PM, Joe Wang wrote:
The changes look good to me.
Best,
Joe
On 3/9/20 1:44 AM, Stephen Colebourne wrote:
Fine by me, but I'm not an OpenJDK Reviewer
Stephen
On Mon, 9 Mar 2020 at 03:05, 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, 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, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
The changes look good to me.
Best,
Joe
On 3/9/20 1:44 AM, Stephen Colebourne wrote:
Fine by me, but I'm not an OpenJDK Reviewer
Stephen
On Mon, 9 Mar 2020 at 03:05, 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, 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, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Fine by me, but I'm not an OpenJDK Reviewer
Stephen
On Mon, 9 Mar 2020 at 03:05, 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, 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, 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
>
>
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
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, 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, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
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, 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, 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
> >>
> >>
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
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, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
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, 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
>
>
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Looks good to me.
Thanks,
Joe
On 3/3/20 10:15 AM, naoto.s...@oracle.com wrote:
Thanks, Joe. Updated:
http://cr.openjdk.java.net/~naoto/8239836/webrev.02/
Naoto
On 3/3/20 8:59 AM, Joe Wang wrote:
On 3/3/20 8:27 AM, naoto.s...@oracle.com wrote:
Hi Joe, thanks for the review.
Are you suggesting something like
isFixedOffset() {
return isFixedOffset;
}
Yes, something like that. It could be initiated while the rules is
constructed. I feel it might be semantically clearer as transitions
indirectly reflect that the offset is fixed. Your call.
Best,
Joe
Naoto
On 3/2/20 11:20 PM, Joe Wang wrote:
Hi Naoto,
The fix would be fine if you want to keep it as is since it does work.
I noticed though that for standard zones (the ones loaded from tz
database), savingsInstantTransitions and standardTransitions are
consistent in that they are both empty for the standard zones, e.g.
Etc/GMT, and not empty for zones with a country/city id, including
countries that don't actually observe DST. This means a check for
one of them is enough for standard zones, which was done in the
current implementation (that is, isFixedOffset() returns
savingsInstantTransitions.length == 0). For custom ZoneRules
created with the "of" method, it would depend on whether they are
set through the relevant parameters (in the test case, the later
was set but the former was empty, that was why isFixedOffset was
true). It would therefore be possible to add and use a transient
boolean field to represent isFixedOffset.
Just my two cents.
-Joe
On 3/2/20 10:37 AM, Roger Riggs wrote:
Looks good.
Give it a day to see if anyone else has comments.
Thanks, Roger
On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote:
Hi Roger, thanks for the review.
On 3/2/20 8:44 AM, Roger Riggs wrote:
Hi Naoto,
look ok.
ZoneRules.java: 488, 644, 761, 791
I'd suggest calling isFixedOffset() instead of repeating the code:
standardTransitions.length == 0 &&
savingsInstantTransitions.length == 0
Modified as suggested:
http://cr.openjdk.java.net/~naoto/8239836/webrev.01/
It should be inlined in cases where the performance matters.
None of those locations is invoked during ZoneRules object
instantiation. So I believe it is OK to replace them with
isFixedOffset().
Naoto
Thanks, Roger
On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Thanks, Joe. Updated:
http://cr.openjdk.java.net/~naoto/8239836/webrev.02/
Naoto
On 3/3/20 8:59 AM, Joe Wang wrote:
On 3/3/20 8:27 AM, naoto.s...@oracle.com wrote:
Hi Joe, thanks for the review.
Are you suggesting something like
isFixedOffset() {
return isFixedOffset;
}
Yes, something like that. It could be initiated while the rules is
constructed. I feel it might be semantically clearer as transitions
indirectly reflect that the offset is fixed. Your call.
Best,
Joe
Naoto
On 3/2/20 11:20 PM, Joe Wang wrote:
Hi Naoto,
The fix would be fine if you want to keep it as is since it does work.
I noticed though that for standard zones (the ones loaded from tz
database), savingsInstantTransitions and standardTransitions are
consistent in that they are both empty for the standard zones, e.g.
Etc/GMT, and not empty for zones with a country/city id, including
countries that don't actually observe DST. This means a check for one
of them is enough for standard zones, which was done in the current
implementation (that is, isFixedOffset() returns
savingsInstantTransitions.length == 0). For custom ZoneRules created
with the "of" method, it would depend on whether they are set through
the relevant parameters (in the test case, the later was set but the
former was empty, that was why isFixedOffset was true). It would
therefore be possible to add and use a transient boolean field to
represent isFixedOffset.
Just my two cents.
-Joe
On 3/2/20 10:37 AM, Roger Riggs wrote:
Looks good.
Give it a day to see if anyone else has comments.
Thanks, Roger
On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote:
Hi Roger, thanks for the review.
On 3/2/20 8:44 AM, Roger Riggs wrote:
Hi Naoto,
look ok.
ZoneRules.java: 488, 644, 761, 791
I'd suggest calling isFixedOffset() instead of repeating the code:
standardTransitions.length == 0 &&
savingsInstantTransitions.length == 0
Modified as suggested:
http://cr.openjdk.java.net/~naoto/8239836/webrev.01/
It should be inlined in cases where the performance matters.
None of those locations is invoked during ZoneRules object
instantiation. So I believe it is OK to replace them with
isFixedOffset().
Naoto
Thanks, Roger
On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
On 3/3/20 8:27 AM, naoto.s...@oracle.com wrote:
Hi Joe, thanks for the review.
Are you suggesting something like
isFixedOffset() {
return isFixedOffset;
}
Yes, something like that. It could be initiated while the rules is
constructed. I feel it might be semantically clearer as transitions
indirectly reflect that the offset is fixed. Your call.
Best,
Joe
Naoto
On 3/2/20 11:20 PM, Joe Wang wrote:
Hi Naoto,
The fix would be fine if you want to keep it as is since it does work.
I noticed though that for standard zones (the ones loaded from tz
database), savingsInstantTransitions and standardTransitions are
consistent in that they are both empty for the standard zones, e.g.
Etc/GMT, and not empty for zones with a country/city id, including
countries that don't actually observe DST. This means a check for one
of them is enough for standard zones, which was done in the current
implementation (that is, isFixedOffset() returns
savingsInstantTransitions.length == 0). For custom ZoneRules created
with the "of" method, it would depend on whether they are set through
the relevant parameters (in the test case, the later was set but the
former was empty, that was why isFixedOffset was true). It would
therefore be possible to add and use a transient boolean field to
represent isFixedOffset.
Just my two cents.
-Joe
On 3/2/20 10:37 AM, Roger Riggs wrote:
Looks good.
Give it a day to see if anyone else has comments.
Thanks, Roger
On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote:
Hi Roger, thanks for the review.
On 3/2/20 8:44 AM, Roger Riggs wrote:
Hi Naoto,
look ok.
ZoneRules.java: 488, 644, 761, 791
I'd suggest calling isFixedOffset() instead of repeating the code:
standardTransitions.length == 0 &&
savingsInstantTransitions.length == 0
Modified as suggested:
http://cr.openjdk.java.net/~naoto/8239836/webrev.01/
It should be inlined in cases where the performance matters.
None of those locations is invoked during ZoneRules object
instantiation. So I believe it is OK to replace them with
isFixedOffset().
Naoto
Thanks, Roger
On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Hi Joe, thanks for the review.
Are you suggesting something like
isFixedOffset() {
return isFixedOffset;
}
Naoto
On 3/2/20 11:20 PM, Joe Wang wrote:
Hi Naoto,
The fix would be fine if you want to keep it as is since it does work.
I noticed though that for standard zones (the ones loaded from tz
database), savingsInstantTransitions and standardTransitions are
consistent in that they are both empty for the standard zones, e.g.
Etc/GMT, and not empty for zones with a country/city id, including
countries that don't actually observe DST. This means a check for one of
them is enough for standard zones, which was done in the current
implementation (that is, isFixedOffset() returns
savingsInstantTransitions.length == 0). For custom ZoneRules created
with the "of" method, it would depend on whether they are set through
the relevant parameters (in the test case, the later was set but the
former was empty, that was why isFixedOffset was true). It would
therefore be possible to add and use a transient boolean field to
represent isFixedOffset.
Just my two cents.
-Joe
On 3/2/20 10:37 AM, Roger Riggs wrote:
Looks good.
Give it a day to see if anyone else has comments.
Thanks, Roger
On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote:
Hi Roger, thanks for the review.
On 3/2/20 8:44 AM, Roger Riggs wrote:
Hi Naoto,
look ok.
ZoneRules.java: 488, 644, 761, 791
I'd suggest calling isFixedOffset() instead of repeating the code:
standardTransitions.length == 0 && savingsInstantTransitions.length
== 0
Modified as suggested:
http://cr.openjdk.java.net/~naoto/8239836/webrev.01/
It should be inlined in cases where the performance matters.
None of those locations is invoked during ZoneRules object
instantiation. So I believe it is OK to replace them with
isFixedOffset().
Naoto
Thanks, Roger
On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Hi Naoto, The fix would be fine if you want to keep it as is since it does work. I noticed though that for standard zones (the ones loaded from tz database), savingsInstantTransitions and standardTransitions are consistent in that they are both empty for the standard zones, e.g. Etc/GMT, and not empty for zones with a country/city id, including countries that don't actually observe DST. This means a check for one of them is enough for standard zones, which was done in the current implementation (that is, isFixedOffset() returns savingsInstantTransitions.length == 0). For custom ZoneRules created with the "of" method, it would depend on whether they are set through the relevant parameters (in the test case, the later was set but the former was empty, that was why isFixedOffset was true). It would therefore be possible to add and use a transient boolean field to represent isFixedOffset. Just my two cents. -Joe On 3/2/20 10:37 AM, Roger Riggs wrote: Looks good. Give it a day to see if anyone else has comments. Thanks, Roger On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote: Hi Roger, thanks for the review. On 3/2/20 8:44 AM, Roger Riggs wrote: Hi Naoto, look ok. ZoneRules.java: 488, 644, 761, 791 I'd suggest calling isFixedOffset() instead of repeating the code: standardTransitions.length == 0 && savingsInstantTransitions.length == 0 Modified as suggested: http://cr.openjdk.java.net/~naoto/8239836/webrev.01/ It should be inlined in cases where the performance matters. None of those locations is invoked during ZoneRules object instantiation. So I believe it is OK to replace them with isFixedOffset(). Naoto Thanks, Roger On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Looks good. Give it a day to see if anyone else has comments. Thanks, Roger On 3/2/20 1:35 PM, naoto.s...@oracle.com wrote: Hi Roger, thanks for the review. On 3/2/20 8:44 AM, Roger Riggs wrote: Hi Naoto, look ok. ZoneRules.java: 488, 644, 761, 791 I'd suggest calling isFixedOffset() instead of repeating the code: standardTransitions.length == 0 && savingsInstantTransitions.length == 0 Modified as suggested: http://cr.openjdk.java.net/~naoto/8239836/webrev.01/ It should be inlined in cases where the performance matters. None of those locations is invoked during ZoneRules object instantiation. So I believe it is OK to replace them with isFixedOffset(). Naoto Thanks, Roger On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Hi Roger, thanks for the review. On 3/2/20 8:44 AM, Roger Riggs wrote: Hi Naoto, look ok. ZoneRules.java: 488, 644, 761, 791 I'd suggest calling isFixedOffset() instead of repeating the code: standardTransitions.length == 0 && savingsInstantTransitions.length == 0 Modified as suggested: http://cr.openjdk.java.net/~naoto/8239836/webrev.01/ It should be inlined in cases where the performance matters. None of those locations is invoked during ZoneRules object instantiation. So I believe it is OK to replace them with isFixedOffset(). Naoto Thanks, Roger On 2/27/20 3:41 PM, 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
Re: [15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
Hi Naoto, look ok. ZoneRules.java: 488, 644, 761, 791 I'd suggest calling isFixedOffset() instead of repeating the code: standardTransitions.length == 0 && savingsInstantTransitions.length == 0 It should be inlined in cases where the performance matters. Thanks, Roger On 2/27/20 3:41 PM, 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
[15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity
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
