bpo-26669: Fix nan arg value error in pytime.c

[hahnlee]

• Make value error message for nan value
  Previously, it did not show error message in arm system
• Add test case for nan value

https://bugs.python.org/issue26669

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

[wkschwartz]

Might be helpful for the error message specifically to mention that the value is NaN.

[pitrou]

@Haypo, do you have any comments about this?

[vstinner]

Ok for "functional" tests", but I also would like to see unit tests on the following _testcapi functions:

• pytime_object_to_time_t
• pytime_object_to_timeval
• pytime_object_to_timespec
• PyTime_FromSeconds
• PyTime_FromSecondsObject
• PyTime_AsSecondsDouble

See CPyTimeTestCase in test_time.py.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

[pitrou]

Hmm... Did @warsaw hack the bot?

[vstinner]

@Sn0wLe0pard: Would you mind to add requested unit tests? Otherwise, I might try to modify your PR to add them myself?

[hahnlee]

@Haypo I'll add test case as soon as possible.

[hahnlee]

@Haypo I'm really sorry for the delay in writing the code.

Summary

System: Ubuntu 16.04 x86
pytime_object_to_timeval got error.
If the result is the same as expected, I will replace the passed test code with the changed _PyTime_FromSecondsObject and _PyTime_ObjectToDenominator.

• pytime_object_to_time_t: pass
• pytime_object_to_timeval : error assert(0 <= *usec && *usec < SEC_TO_US);
• pytime_object_to_timespec: error assert(0 <= *nsec && *nsec < SEC_TO_NS);
• PyTime_FromSecondsObject: pass (When add some C code)
• PyTime_FromSeconds: TypeError: integer argument expected, got float
• PyTime_FromSecondsObject: TypeError: integer argument expected, got float

Result

I tested the following code in _check_rounding.

# test nan
for time_rnd, _ in ROUNDING_MODES:
    with self.assertRaises(ValueError):
        pytime_converter(float('nan'), time_rnd)

Here are the results:

Python/pytime.c:116: _PyTime_DoubleToDenominator: Assertion `0.0 <= floatpart && floatpart < denominator' failed.

...so I tested one by one.
pytime_object_to_time_t: pass
pytime_object_to_timeval: error, pytime.c:116
pytime_object_to_timespec: error, pytime.c:116

PyTime_FromSeconds: error TypeError: integer argument expected, got float (did not use time_rnd)
PyTime_AsSecondsDouble: has same error PyTime_FromSecondsObject (did not use time_rnd)

And PyTime_FromSecondsObject: did not raise
_PyTime_FromSecondsObject call _PyTime_FromObject and _PyTime_FromObject has same logic at _PyTime_ObjectToTime_t (check float but do not check nan)
So I changed _PyTime_FromObject and passed test for _PyTime_FromSecondsObject.

In case pytime_object_to_timeval and pytime_object_to_timespec they call _PyTime_ObjectToDenominator and _PyTime_ObjectToDenominator has PyFloat_AsDouble too

I add Py_IS_NAN(d) at _PyTime_ObjectToDenominator

but pytime_object_to_timeval and pytime_object_to_timespec got assert error
it mean in case _PyTime_ObjectToDenominator, *obj float('nan') has problem in usec or nsec

i think PyTime_FromSeconds and PyTime_AsSecondsDouble seem reasonable.
but i'm not sure pytime_object_to_timeval and pytime_object_to_timespec has reasonable error.

If the result is the same as expected, I will replace the passed test code with the changed _PyTime_FromSecondsObject and _PyTime_ObjectToDenominator.

[hahnlee]

pytime_object_to_timeval also has same error on macOS sierra

[hahnlee]

Oh... in case NaN numerator is zero. I add some code for numerator zero. then it pass all test cases.

[vstinner]

Ok, it now LGTM!

[miss-islington]

🐍🍒⛏🤖 Thanks @Sn0wLe0pard for the PR, and @Haypo for merging it 🌮🎉.I'm working now to backport this PR to: 3.6.

[bedevere-bot]

GH-3467 is a backport of this pull request to the 3.6 branch.