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

[naoto.sato at oracle.com]

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