Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions Lib/test/test_complex.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import errno
import unittest
import sys
from test import support
from test.support import import_helper
from test.support.testcase import ComplexesAreIdenticalMixin
from test.support.numbers import (
VALID_UNDERSCORE_LITERALS,
Expand All @@ -9,6 +11,7 @@

from random import random
from math import isnan, copysign
import cmath
import operator

INF = float("inf")
Expand Down Expand Up @@ -789,8 +792,30 @@ def test_abs(self):
for num in nums:
self.assertAlmostEqual((num.real**2 + num.imag**2) ** 0.5, abs(num))

for x in 0.0, -0.0, INF, -INF, NAN:
for y in 0.0, -0.0, INF, -INF, NAN:
with self.subTest(x=x, y=y):
z = complex(x, y)
r = abs(z)
if cmath.isfinite(z):
self.assertFloatsAreIdentical(r, 0.0)
elif cmath.isinf(z):
self.assertEqual(r, INF)
else:
self.assertTrue(cmath.isnan(z))
self.assertTrue(isnan(r))

self.assertRaises(OverflowError, abs, complex(DBL_MAX, DBL_MAX))

def test_abs_errno_handling(self):
_testcapi = import_helper.import_module('_testcapi')
z = complex('nan')
_testcapi.set_errno(errno.ERANGE)
try:
self.assertTrue(isnan(abs(z)))
finally:
_testcapi.set_errno(0)

def test_repr_str(self):
def test(v, expected, test_fn=self.assertEqual):
test_fn(repr(v), expected)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Correct ``errno`` handling in ``abs(complex)``. Patch by Sergey B
Kirpichev.
18 changes: 10 additions & 8 deletions Objects/complexobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ c_powi(Py_complex x, long n)
double
_Py_c_abs(Py_complex z)
{
/* sets errno = ERANGE on overflow; otherwise errno = 0 */
/* sets errno = ERANGE on overflow */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer setting errno to 0 sounds risky. With this change, multiple cmath function now depends on the current errno value: polar() and isclose().

Since _Py_c_abs() is our custom API, why not change its API to report the error, rather than relying on the global variable errno?

For example, change the API to int _Py_c_abs(Py_complex z, double *result): set *result and return 0 on success, return -1 on error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, _Py_c_abs() is documented.
An external API should not set errno to zero when the function succeeds. (So, I don't think we should change the documentation to say it does. We should change the implementation.)

The function polar() sets errno = 0 before calling _Py_c_abs(z), so it's OK.
The function isclose() does not read errno, so it's OK.

I suggest: https://github.com/hpkfft/cpython/blob/erange/Objects/complexobject.c#L380-L417
This keeps the documented API, but adds a new function c_abs() for internal use.
If c_abs() is useful in cmathmodule.c, maybe it needs a better name (and, of course, cannot be static).
This can be done as part of #156145

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I forgot that _Py_c_abs() is part of the public C API (but is private).

double result;

if (!isfinite(z.real) || !isfinite(z.imag)) {
Expand All @@ -376,23 +376,20 @@ _Py_c_abs(Py_complex z)
NaN. */
if (isinf(z.real)) {
result = fabs(z.real);
errno = 0;
return result;
}
if (isinf(z.imag)) {
result = fabs(z.imag);
errno = 0;
return result;
}
/* either the real or imaginary part is a NaN,
and neither is infinite. Result should be NaN. */
return Py_NAN;
}
result = hypot(z.real, z.imag);
if (!isfinite(result))
if (!isfinite(result)) {
errno = ERANGE;
else
errno = 0;
}
return result;
}

Expand Down Expand Up @@ -796,8 +793,13 @@ static PyObject *
complex_abs(PyObject *op)
{
PyComplexObject *v = _PyComplexObject_CAST(op);
double result = _Py_c_abs(v->cval);
if (errno == ERANGE) {
double result;

result = hypot(v->cval.real, v->cval.imag);
/* Testing FE_OVERFLOW floating-point exception is slow. */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you mention in the comment that errno is not used on purpose, maybe with a reference to gh-155526?

if (isfinite(v->cval.real) && isfinite(v->cval.imag)
&& !isfinite(result))
{
PyErr_SetString(PyExc_OverflowError,
"absolute value too large");
return NULL;
Expand Down
Loading