STINNER Victor <vstin...@redhat.com> added the comment:
I looked at Pablo's PR 6149 and I'm surprised by the size of the code just to
convert a Python object to a C long!
---
if (PyFloat_Check(arg)) {
PyObject *lx;
double dx = PyFloat_AS_DOUBLE((PyFloatObject *)arg);
if (!(Py_IS_FINITE(dx) && dx == floor(dx))) {
PyErr_SetString(PyExc_ValueError,
"factorial() only accepts integral values");
return NULL;
}
lx = PyLong_FromDouble(dx);
if (lx == NULL)
return NULL;
x = PyLong_AsLongAndOverflow(lx, &overflow);
Py_DECREF(lx);
}
else {
pyint_form = PyNumber_Index(arg);
if (pyint_form == NULL) {
return NULL;
}
x = PyLong_AsLongAndOverflow(pyint_form, &overflow);
Py_DECREF(pyint_form);
}
if (x == -1 && PyErr_Occurred()) {
return NULL;
}
else if (overflow == 1) {
PyErr_Format(PyExc_OverflowError,
"factorial() argument should not exceed %ld",
LONG_MAX);
return NULL;
}
else if (overflow == -1 || x < 0) {
PyErr_SetString(PyExc_ValueError,
"factorial() not defined for negative values");
return NULL;
}
---
Do we really need 37 lines of C code? Is it really important to accept integral
float? What's wrong with factorial(int(d)) for example?
PR 6149 LGTM, but since I was not involved in the discussion, I would prefer if
someone else who has been involved would approve the change: Tal already
approved it, but I saw some complains, I'm not 100% sure that we reached a full
agreement.
Note: Pablo is a core developer and so he can merge his PR, I'm only asking to
*approve* the change, not to merge it ;-)
Thanks in advance.
Note2: I'm still mentoring Pablo after he became a core, and I require him to
ask me before merging any change.
----------
nosy: +vstinner
_______________________________________
Python tracker <rep...@bugs.python.org>
<https://bugs.python.org/issue33083>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
