Use Py_complex_protected AC converter for cmath.rect()

[skirpichev]

No description provided.

[skirpichev]

CC @zware

[vstinner]

AC generates the following code in cmath_acos():

    /* modifications for z */
    errno = 0;

Then cmath_acos_impl() sets again errno:

    errno = 0;

There are also redundant errno = 0; statements in cmath_rect_impl():

static Py_complex
cmath_rect_impl(PyObject *module, double r, double phi)
{
    Py_complex z;
    errno = 0;

    /* deal with special values */
    if (!isfinite(r) || !isfinite(phi)) {
        ...
        if (r != 0. && !isnan(r) && isinf(phi))
            errno = EDOM;
        else
            errno = 0;
    }
    else if (phi == 0.0) {
        ...
        errno = 0;
    }
    else {
        ...
        errno = 0;
    }
    return z;
}

Maybe the first errno = 0; in _impl() functions can be removed.

[skirpichev]

Maybe the first errno = 0; in _impl() functions can be removed.

No, here it's on purpose. Your remark might have sense for Py_complex_protected converter.

Though, I don't think that it could be removed here as well. Most functions ignore errno, yes, but not all: cmath_log10_impl is an example.

There are also redundant errno = 0; statements in cmath_rect_impl()

That's true. Another example could be cmath_log_impl(), if converted to Py_complex_protected.

We also could utilize bool return converter here and in the math module.

I can prepare a small refactoring PR, if you think it worth.

[skirpichev]

Here patch:

diff --git a/Modules/cmathmodule.c b/Modules/cmathmodule.c
index f6e1475b00e..d12c2a87e6c 100644
--- a/Modules/cmathmodule.c
+++ b/Modules/cmathmodule.c
@@ -23,11 +23,6 @@ module cmath
 /*[clinic end generated code: output=da39a3ee5e6b4b0d input=308d6839f4a46333]*/
 
 /*[python input]
-class Py_complex_protected_converter(Py_complex_converter):
-    def modify(self):
-        return 'errno = 0;'
-
-
 class Py_complex_protected_return_converter(CReturnConverter):
     type = "Py_complex"
 
@@ -47,7 +42,7 @@ else {
 }
 """.strip())
 [python start generated code]*/
-/*[python end generated code: output=da39a3ee5e6b4b0d input=8b27adb674c08321]*/
+/*[python end generated code: output=da39a3ee5e6b4b0d input=277d5217448652ab]*/
 
 #if (FLT_RADIX != 2 && FLT_RADIX != 16)
 #error "Modules/cmathmodule.c expects FLT_RADIX to be 2 or 16"
@@ -175,7 +170,7 @@ static Py_complex acos_special_values[7][7] = {
 /*[clinic input]
 cmath.acos -> Py_complex_protected
 
-    z: Py_complex_protected
+    z: Py_complex
     /
 
 Return the arc cosine of z.
@@ -183,7 +178,7 @@ Return the arc cosine of z.
 
 static Py_complex
 cmath_acos_impl(PyObject *module, Py_complex z)
-/*[clinic end generated code: output=40bd42853fd460ae input=bd6cbd78ae851927]*/
+/*[clinic end generated code: output=40bd42853fd460ae input=fa67fb00742399e7]*/
 {
     Py_complex s1, s2, r;
 
@@ -652,6 +647,7 @@ cmath_log10_impl(PyObject *module, Py_complex z)
     Py_complex r;
     int errno_save;
 
+    errno = 0;
     r = c_log(z);
     errno_save = errno; /* just in case the divisions affect errno */
     r.real = r.real / M_LN10;
@@ -1074,7 +1070,6 @@ cmath_rect_impl(PyObject *module, double r, double phi)
 /*[clinic end generated code: output=74ff3d17585f3388 input=50e60c5d28c834e6]*/
 {
     Py_complex z;
-    errno = 0;
 
     /* deal with special values */
     if (!isfinite(r) || !isfinite(phi)) {
@@ -1120,42 +1115,45 @@ cmath_rect_impl(PyObject *module, double r, double phi)
 
 /*[clinic input]
 @permit_long_summary
-cmath.isfinite = cmath.polar
+cmath.isfinite -> bool
+
+    z: Py_complex
+    /
 
 Return True if both the real and imaginary parts of z are finite, else False.
 [clinic start generated code]*/
 
-static PyObject *
+static int
 cmath_isfinite_impl(PyObject *module, Py_complex z)
-/*[clinic end generated code: output=ac76611e2c774a36 input=e224f5c36d94f5da]*/
+/*[clinic end generated code: output=a6fc66e225b67f3e input=48df1ca085dd98bc]*/
 {
-    return PyBool_FromLong(isfinite(z.real) && isfinite(z.imag));
+    return isfinite(z.real) && isfinite(z.imag);
 }
 
 /*[clinic input]
-cmath.isnan = cmath.polar
+cmath.isnan = cmath.isfinite
 
 Checks if the real or imaginary part of z not a number (NaN).
 [clinic start generated code]*/
 
-static PyObject *
+static int
 cmath_isnan_impl(PyObject *module, Py_complex z)
-/*[clinic end generated code: output=e7abf6e0b28beab7 input=71799f5d284c9baf]*/
+/*[clinic end generated code: output=e05d542da2445031 input=52c2e905049e6db1]*/
 {
-    return PyBool_FromLong(isnan(z.real) || isnan(z.imag));
+    return isnan(z.real) || isnan(z.imag);
 }
 
 /*[clinic input]
-cmath.isinf = cmath.polar
+cmath.isinf = cmath.isfinite
 
 Checks if the real or imaginary part of z is infinite.
 [clinic start generated code]*/
 
-static PyObject *
+static int
 cmath_isinf_impl(PyObject *module, Py_complex z)
-/*[clinic end generated code: output=502a75a79c773469 input=363df155c7181329]*/
+/*[clinic end generated code: output=6831567fcb77dc69 input=a1cc46c97a6f7a2a]*/
 {
-    return PyBool_FromLong(isinf(z.real) || isinf(z.imag));
+    return isinf(z.real) || isinf(z.imag);
 }
 
 /*[clinic input]

[vstinner]

Your remark might have sense for Py_complex_protected converter.

My remark is only about the two functions using it.

Maybe errno = 0; can be replace with assert(errno == 0); in the impl() functions. It would keep the function readable (no need to understand AC internals) and avoid redundant initialization.

Well, it's a remark, not a big deal. I'm not even sure if it's worth it to change it.