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

[Joe Wang]

Looks good to me.

Thanks,
Joe

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