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

[Joe Wang]

On 3/3/20 8:27 AM, naoto.sato at 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.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
>>>>>>
>>>>>>
>>>>>
>>>
>>