Author mark.dickinson
Recipients amaury.forgeotdarc, benjamin.peterson, brett.cannon, dmalcolm, ilblackdragon, jcea, larry, mark.dickinson, vstinner
Date 2013-03-31.11:56:21
SpamBayes Score -1.0
Marked as misclassified Yes
Message-id <1364730981.86.0.331979428697.issue17206@psf.upfronthosting.co.za>
In-reply-to
Content
Thanks for the updated patch.  Some comments:

- In the tests, your functions should have return type PyObject* rather than void;  you can use Py_RETURN_NONE as a convenient way to return something of the correct type.  E.g.:

static PyObject *
test_decref_doesnt_leak(PyObject* self)
{
    Py_DECREF(PyLong_FromLong(0));
    Py_RETURN_NONE;
}

- Unfortunately, the test above *will* leak:  even though your patch fixes Py_XDECREF, Py_DECREF will still evaluate the argument multiple times.  So we get, for example:

iwasawa:cpython mdickinson$ ./python.exe -m test -R:: test_capi
[1/1] test_capi
beginning 9 repetitions
123456789
.........
test_capi leaked [1, 1, 1, 1] references, sum=4
1 test failed:
    test_capi

- It would be great to have tests for Py_INCREF and Py_XINCREF, too.

- The patch introduces two extra blank lines before Py_CLEAR.

- Not specifically about the patch, but: there's a comment in object.h that reads:

"""
*** WARNING*** The Py_DECREF macro must have a side-effect-free argument
since it may evaluate its argument multiple times.  (The alternative
would be to mace it a proper function or assign it to a global temporary
variable first, both of which are slower; and in a multi-threaded
environment the global variable trick is not safe.)
"""

I don't understand why the author of that comment ruled out the possibility of a local temporary variable.  Anyone have any ideas?  A constraint of some flavours of pre-ANSI C, perhaps?
History
Date User Action Args
2013-03-31 11:56:21mark.dickinsonsetrecipients: + mark.dickinson, brett.cannon, jcea, amaury.forgeotdarc, vstinner, larry, benjamin.peterson, dmalcolm, ilblackdragon
2013-03-31 11:56:21mark.dickinsonsetmessageid: <1364730981.86.0.331979428697.issue17206@psf.upfronthosting.co.za>
2013-03-31 11:56:21mark.dickinsonlinkissue17206 messages
2013-03-31 11:56:21mark.dickinsoncreate