[15] RFR: 8239836: ZoneRules.of() doesn't check transitionList/standardOffsetTL arguments validity

[Joe Wang]

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.sato at 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.sato at 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
>>>>
>>>>
>>>
>