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
