(original) (raw)

After a huge delay, I finally found the time to implement the PEP 485 isclose() function, in C. I tihnk it's time for some review. I appologise for the fact that I have little experience with C, and haven't used the raw C API for years, but it's a pretty simple function, and there's lots of code to copy, so I think it's in OK shape. I hav not yet integrated it with the cPyton source code -- it belongs in mathmodule.c, but I found it easier to put it in a separate module while figuring it out.

There is a test module in:
test\_isclose\_c.py

and it can be built with:

python3 setup.py build\_ext --inplace

Let me know if I should restructure it or put it anywhere else before it gets reviewed but in the meantime, i welcome any feedback.

Thanks,

-Chris

A few questions I have off the bat:

C-API (and plain C) questions:
=============================

\* Is there a better way to create a False or True than::

PyBool\_FromLong(0) and PyBool\_FromLong(1)

\* Style question: should I put brackets in an if clause that has only one line?::

if (some\_check) {
just\_this\_one\_expression
}

\* I can't find docs for PyDoc\_STRVAR: but it looks like it should use it -- how?

\* I'm getting a warning in my PyMethodDef clause::

static PyMethodDef IsCloseMethods\[\] = {
{"isclose", isclose\_c, METH\_VARARGS | METH\_KEYWORDS,
"determine if two floating point numbers are close"},
{NULL, NULL, 0, NULL} /\* Sentinel \*/
};

is\_close\_module.c:61:17: warning: incompatible pointer types initializing
'PyCFunction' (aka 'PyObject \*(\*)(PyObject \*, PyObject \*)') with an
expression of type 'PyObject \*(PyObject \*, PyObject \*, PyObject \*)'
\[-Wincompatible-pointer-types\]
{"isclose", isclose\_c, METH\_VARARGS | METH\_KEYWORDS,

but it seems to be working -- and I've copied, as well as I can, the examples.

Functionality Questions:
========================

\* What do do with other numeric types?
- Integers cast fine...
- Decimal and Fraction cast fine, too -- but precision is presumably lost.
- Complex ? -- add it to cmath?


\* It's raising an Exception for negative tolerances: which don't make sense,
but don't really cause harm either (fabs() is used anyway). I can't say I recall why I did that
for the python prototype, but I reproduced in the C version. Should I?

\* What about zero tolerance? should equal values still be considered close? They are now, and tests reflect that.