gh-143429: Use compile-time NaN encoding detection for test_struct

[chenx97]

Description

This PR uses C macros to detect NaN encoding schemes for Lib/test/test_struct.py, instead of detecting the host machine.

math.nan depends on toolchain settings for a MIPS build, as setting -mnan=legacy and -mnan=2008 result in different __builtin_nan behavior for GCC.

Tests

• Ran the official test suite: ./python -m test test_struct (Passed).

• Issue: test_half_float in test_struct fails on mips #143429

[skirpichev]

In principle, this looks good for me. But I would prefer a different solution, see issue thread.

CC @vstinner

[vstinner]

Would it be possible to move this change to Modules/_testcapi/float.c?

[vstinner]

LGTM. I just suggest moving the C change to Modules/_testcapi/float.c.

[picnixz]

Only use this import in the test otherwise the entire module will be skiped if _testcapi doesn't exist.

[vstinner]

Here you can use:

try:
    import _testcapi
except ImportError:
    _testcapi = None

And later check if _testcapi is None.

[skirpichev]

Maybe just move test_half_float() test to the test_capi/test_float.py?

[vstinner]

I disagree, these tests are not testing the C API.

[skirpichev]

Right. But this code essentially just tests PyFloat_Pack2/UnPack2() via the struct module API.

Then, maybe add a configure test and use the sysconfig module to query this stuff? Though, I think it's fine to skip just this one test if _testcapi is missing.

[vstinner]

Or just move _testcapi = import_helper.import_module('_testcapi') in test_half_float().

[picnixz]

Since _testcapi is only used in that specific test, it should only be imported inside it and possibly skipped. import_helper.import_module already raises SkipTest if it can't import the module so there is no real need to do try-except with _testcapi is None + decorator (unless at some point we need lots of _testcapi accesses...)

[picnixz]

Forgot to explicitly request changes.

[bedevere-app]

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 have made the requested changes; please review again. 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.

[skirpichev]

"parisc" looks rather cryptic for me. Maybe "msb_is_signaling" as a value? Or what about some boolean flag like nan_msb_is_signaling?

Suggested change

	#if (defined(__mips__) && !defined(__mips_nan2008)) || defined(__hppa__)
	PyModule_Add(m, "nan_encoding", PyUnicode_FromString("parisc"));
	#else
	PyModule_Add(m, "nan_encoding", PyUnicode_FromString("regular"));
	#endif
	#if (defined(__mips__) && !defined(__mips_nan2008)) || defined(__hppa__)
	PyModule_Add(m, "nan_msb_is_signaling", PyBool_FromLong(1));
	#else
	PyModule_Add(m, "nan_msb_is_signaling", PyBool_FromLong(0));
	#endif

stale

[skirpichev]

@chenx97, could you check also test_pack_unpack_roundtrip_for_nans() in Lib/test/test_capi/test_float.py?

Is it enabled on your system? If so, it should fail too. If not, I suspect you could increase number of samples (10 for now) in this test.

[chenx97]

@chenx97, could you check also test_pack_unpack_roundtrip_for_nans() in Lib/test/test_capi/test_float.py?

Is it enabled on your system? If so, it should fail too. If not, I suspect you could increase number of samples (10 for now) in this test.

I think the tests are expected to pass on mips64, as the parisc handling logic only runs for the 32-bit environment. Legacy mips32 fails here, as you guessed.

[skirpichev]

the parisc handling logic only runs for the 32-bit environment

Ah, you are right. (I just did a quick grepping and didn't look on the function code.)

Still, I think we could use new module variable also here, instead of the platform check.

[vstinner]

LGTM. It's better with _testcapi.nan_msb_is_signaling.