RFR(XS): 8169003: LogDecorations.iso8601_utctime_test fails if numeric locale uses ", " as separator between integer and fraction part

[Kirill Zhaldybin]

Igor,

Thank you for review!

Regards, Kirill

On 28.11.2016 15:19, Igor Ignatyev wrote:
> Hi Kirill,
>
> looks good to me, thanks for fixing that.
>
> Cheers,
> — Igor
>
>> On Nov 28, 2016, at 1:06 PM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
>>
>> Hi,
>>
>>
>> On 11/25/2016 06:23 PM, Kirill Zhaldybin wrote:
>>> Marcus,
>>>
>>> Here are a new webrev: http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8169003/webrev.01/
>> Looks ok.
>>
>> Thanks,
>> Marcus
>>
>>> I addressed your comment about "if-case for the Z suffix".
>>>
>>> Could you please let me know your opinion?
>>>
>>> Thank you.
>>>
>>> Regards, Kirill
>>>
>>> On 24.11.2016 17:35, Marcus Larsson wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 11/22/2016 02:24 PM, Kirill Zhaldybin wrote:
>>>>> Marcus,
>>>>>
>>>>> Thank you for prompt reply!
>>>>>
>>>>> Could you please read comments inline?
>>>>> I'm looking forward to your reply.
>>>>>
>>>>> Thank you.
>>>>>
>>>>> Regards, Kirill
>>>>>
>>>>> On 22.11.2016 15:32, Marcus Larsson wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> On 2016-11-21 17:38, Kirill Zhaldybin wrote:
>>>>>>> Marcus,
>>>>>>>
>>>>>>> Thank you for reviewing the fix!
>>>>>>>>> WebRev:
>>>>>>>>> http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8169003/webrev.00/
>>>>>>>> ISO8601 says the decimal point can be either '.' or ',' so the test
>>>>>>>> should accept either. You could let sscanf read out the decimal
>>>>>>>> point as a character and just verify that it is one of the two.
>>>>>>>>
>>>>>>>> In the UTC test you changed ASSERT_GT to ASSERT_EQ, which means
>>>>>>>> that we won't accept "Z" suffixed strings. Please revert that.
>>>>>>> I agree that ISO8601 could add "Z" to time (and as far as I
>>>>>>> understand date/time without delimiters is legal too) but these are
>>>>>>> the unit tests.
>>>>>>> Hence they cover the existing code and they should pass only if the
>>>>>>> result corresponds to existing code and fail otherwise.
>>>>>>> The current code from os::iso8601_time format date/time string
>>>>>>> %04d-%02d-%02dT%02d:%02d:%02d.%03d%c%02d%02d so we should not
>>>>>>> consider any other format as valid.
>>>>>>>
>>>>>>> Could you please let me know your opinion?
>>>>>> I think the test should verify the intended behavior, not the
>>>>>> implementation. If we refactor or change something in iso8601_time()
>>>>>> we shouldn't be failing the test if it still conforms to ISO8601, IMO.
>>>>> I would agree with you if we were talking about a functional test. But
>>>>> since it is an unit test I think we should keep it as close to
>>>>> implementation as possible.
>>>>> If the implementation is changed unintentionally the test fails and
>>>>> signals us that something is broken.
>>>>> If it is an intentional change the test must be updated correspondingly.
>>>> I still think it's unnecessary noise, but if you insist I'm fine with it.
>>>>
>>>> If we're not going to accept anything else than the current
>>>> implementation then you should also remove the if-case for the Z suffix,
>>>> since the test will fail for that anyway.
>>>>
>>>> Thanks,
>>>> Marcus
>>>>
>>>>>> Thanks,
>>>>>> Marcus
>>>>>>
>>>>>>> Thank you.
>>>>>>>
>>>>>>> Regards, Kirill
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Marcus
>>>>>>>>
>>>>>>>>> CR: https://bugs.openjdk.java.net/browse/JDK-8169003
>>>>>>>>>
>>>>>>>>> Thank you.
>>>>>>>>>
>>>>>>>>> Regards, Kirill