python / cpython

The Python programming language
https://www.python.org
Other
63.16k stars 30.24k forks source link

Remove remnants of support for non-IEEE 754 systems from cmathmodule.c #121268

Closed skirpichev closed 2 months ago

skirpichev commented 3 months ago

Feature or enhancement

Proposal:

Proposed patch:

diff --git a/Modules/cmathmodule.c b/Modules/cmathmodule.c
index bf86a211bc..591442334e 100644
--- a/Modules/cmathmodule.c
+++ b/Modules/cmathmodule.c
@@ -185,15 +185,8 @@ cmath_acos_impl(PyObject *module, Py_complex z)
     if (fabs(z.real) > CM_LARGE_DOUBLE || fabs(z.imag) > CM_LARGE_DOUBLE) {
         /* avoid unnecessary overflow for large arguments */
         r.real = atan2(fabs(z.imag), z.real);
-        /* split into cases to make sure that the branch cut has the
-           correct continuity on systems with unsigned zeros */
-        if (z.real < 0.) {
-            r.imag = -copysign(log(hypot(z.real/2., z.imag/2.)) +
-                               M_LN2*2., z.imag);
-        } else {
-            r.imag = copysign(log(hypot(z.real/2., z.imag/2.)) +
-                              M_LN2*2., -z.imag);
-        }
+        r.imag = -copysign(log(hypot(z.real/2., z.imag/2.)) +
+                           M_LN2*2., z.imag);
     } else {
         s1.real = 1.-z.real;
         s1.imag = -z.imag;
@@ -386,11 +379,7 @@ cmath_atanh_impl(PyObject *module, Py_complex z)
         */
         h = hypot(z.real/2., z.imag/2.);  /* safe from overflow */
         r.real = z.real/4./h/h;
-        /* the two negations in the next line cancel each other out
-           except when working with unsigned zeros: they're there to
-           ensure that the branch cut has the correct continuity on
-           systems that don't support signed zeros */
-        r.imag = -copysign(Py_MATH_PI/2., -z.imag);
+        r.imag = copysign(Py_MATH_PI/2., z.imag);
         errno = 0;
     } else if (z.real == 1. && ay < CM_SQRT_DBL_MIN) {
         /* C99 standard says:  atanh(1+/-0.) should be inf +/- 0i */

Removing this legacy was proposed in #102067, but before merging #31790. Maybe now that change does make sense?

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

https://github.com/python/cpython/pull/102067#issuecomment-1437850117

Linked PRs

serhiy-storchaka commented 2 months ago

For reference, the original code was added in 6f34109384f3a78d5f4f8bdd418a89caca19631e (ported to Python 3 in 53876d9cd8a67d9e67772e082deab92a598f74b3).

vstinner commented 2 months ago

Fixed by change https://github.com/python/cpython/commit/b6e745a27e9c98127acee436e4855066c58b7a3b.

serhiy-storchaka commented 2 months ago

I would like to hear opinions of @tiran and @mdickinson about this change.