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

Marcus Larsson marcus.larsson at oracle.com
Tue Nov 22 12:32:21 UTC 2016


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.


> Thank you.
> Regards, Kirill
>> Thanks,
>> Marcus
>>> CR: https://bugs.openjdk.java.net/browse/JDK-8169003
>>> Thank you.
>>> Regards, Kirill

More information about the hotspot-dev mailing list