bpo-45412: Move _Py_SET_53BIT_PRECISION_START to pycore_pymath.h #28882
Conversation
|
Since my PR only changes private macros, I didn't document my changes (no NEWS entry). |
Move the following macros to the internal C API, to pycore_pymath.h: * _Py_SET_53BIT_PRECISION_HEADER * _Py_SET_53BIT_PRECISION_START * _Py_SET_53BIT_PRECISION_END PEP 7: add braces to if in these macros. Move also _Py_get_387controlword() and _Py_set_387controlword() definitions to pycore_pymath.h. These functions are no longer exported. pystrtod.c now includes pycore_pymath.h.
| @@ -327,86 +327,25 @@ extern "C" { | |||
| * | |||
| * #define HAVE_PY_SET_53BIT_PRECISION 1 | |||
| * | |||
| * and also give appropriate definitions for the following three macros: | |||
There was a problem hiding this comment.
@vstinner Why did you remove this portion of the comment? The comment now no longer makes any sense: defining HAVE_PY_SET_53BIT_PRECISION isn't enough to magically make dtoa.c work if you don't have 53-bit precision.
There was a problem hiding this comment.
I didn't remove it, I moved it to the internal pycore_pymath.h header. I'm trying to not mention implementation details and private functions in the public header files.
There was a problem hiding this comment.
Yes, but the remaining comment in this file is now no longer valid. It says that all you have to do is define HAVE_PY_SET_53BIT_PRECISION. That doesn't work.
There was a problem hiding this comment.
I'm trying to not mention implementation details and private functions in the public header files.
If someone is trying to port CPython to a new platform - say one that still uses 80-bit extended precision, then they may need to define these macros. In that case, where should they put those definitions? Is pyport.h no longer the right place for this sort of thing?
There was a problem hiding this comment.
Before my change, it wasn't possible to define macros like _Py_SET_53BIT_PRECISION_START before including Python.h: it would fail since Python.h always define them. It's different than the Py_NAN macro which is not redefined if it's already defined. My change doesn't change that.
If someone is trying to port CPython to a new platform, they should patch the Python header file. First maintain a patch downstream, and than propose a change upstream.
I don't understand well your idea to port Python to a new platform without modifying header files. Would it mean that each C extensions would have to define macros before including Python.h?
If someone has to modify header files, is it an issue that these macros are now in Include/internal/pycore_pymath.h rather than Include/pyport.h?
There was a problem hiding this comment.
But please could you fix the comment that your change broke?
Apologies. I'm being unreasonably grumpy here. I'll aim to make a PR to fix this sometime this week.
There was a problem hiding this comment.
I would appreciate if you can write it since I'm not sure which changes you expect exactly :-)
Well, it would be better to move HAVE_PY_SET_53BIT_PRECISION in pycore_pymath.h. I kept it in pyport.h since it's a public macro.
Does it make sense to expose HAVE_PY_SET_53BIT_PRECISION in the public C API whereas the actual implementation is in the internal C API?
There was a problem hiding this comment.
Does it make sense to expose HAVE_PY_SET_53BIT_PRECISION in the public C API whereas the actual implementation is in the internal C API?
Not really - all four of these definitions really belong in the same place.
To try to explain: dtoa.c requires that floating-point operations are performed in 53-bit precision. If you have a machine where an x87 FPU is being used for floating-point, and where the precision is set to 64 bits rather than 53 bits, dtoa.c therefore can't be used.
If you want to fix this so that dtoa.c can be used, so that you can get the nice short floating-point representation that most people are used to instead of the "legacy" representation, you have to:
- Define the three
_Py_SET_53BIT_*macros appropriately for your machine (which can't be done portably), so that Python knows how to temporarily set the FPU precision to 53 bits - Then, once you've done step 1, you can set
HAVE_PY_SET_53BIT_PRECISIONto tell Python that it's safe to usedtoa.c.
The current comment in pyport.h now says that if you want to use dtoa.c on one of these problem machines, all you have to do is set HAVE_PY_SET_53BIT_PRECISION, and you're done. And that's just not true: you also have to define the three _PY_SET_53BIT_* macros properly. If you read the previous comment (i.e., the comment as it was before this PR was merged), it explains that you have to carry out both these steps, not just one. That's why I'm saying that after the changes in this PR, the comment is wrong.
The four definitions really do belong all together.
There was a problem hiding this comment.
I created #31171 to move again all code together. I avoided that previously since the implementation is a little bit tricky (let's discuss it there ;-)).
There was a problem hiding this comment.
I merged my PR: 9bbdde2
Move the following macros to the internal C API, to pycore_pymath.h:
PEP 7: add braces to if in these macros.
Move also _Py_get_387controlword() and _Py_set_387controlword()
definitions to pycore_pymath.h.
pystrtod.c now includes pycore_pymath.h.
https://bugs.python.org/issue45412