Author vstinner
Recipients John.Yeung, facundobatista, mark.dickinson, rhettinger, serhiy.storchaka, taleinat, terry.reedy, tim.peters, vstinner
Date 2018-07-31.09:59:08
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1533031148.38.0.56676864532.issue33083@psf.upfronthosting.co.za>
In-reply-to
Content
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.
History
Date User Action Args
2018-07-31 09:59:08vstinnersetrecipients: + vstinner, tim.peters, rhettinger, terry.reedy, facundobatista, mark.dickinson, taleinat, serhiy.storchaka, John.Yeung
2018-07-31 09:59:08vstinnersetmessageid: <1533031148.38.0.56676864532.issue33083@psf.upfronthosting.co.za>
2018-07-31 09:59:08vstinnerlinkissue33083 messages
2018-07-31 09:59:08vstinnercreate