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

[naoto.sato at oracle.com]

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