gh-128509: Add PyUnstable_IsImmortal for finding immortal objects
#129182
Conversation
Co-authored-by: Victor Stinner <[email protected]>
Co-authored-by: Victor Stinner <[email protected]>
|
I think @encukou's review would also be helpful. |
Modules/_testcapi/object.c
Outdated
| static PyObject * | ||
| is_immortal(PyObject *self, PyObject *op) | ||
| { | ||
| return PyBool_FromLong(PyUnstable_IsImmortal(op)); |
There was a problem hiding this comment.
| return PyBool_FromLong(PyUnstable_IsImmortal(op)); | |
| NULLABLE(op) | |
| return PyLong_FromLong(PyUnstable_IsImmortal(op)); |
This allows to test with NULL, and check if the result is not 0 or 1.
There was a problem hiding this comment.
I think it makes sense to crash with NULL.
Lib/test/test_capi/test_immortal.py
Outdated
| for non_immortal in non_immortals: | ||
| with self.subTest(non_immortal=non_immortal): | ||
| self.assertFalse(_testcapi.is_immortal(non_immortal)) | ||
|
|
There was a problem hiding this comment.
Does _testcapi.is_immortal(NULL) crash? If yes, add a comment, otherwise add a test.
There was a problem hiding this comment.
If yes, add a comment
And ideally an assert to the implementation (as “executable documentation”).
Lib/test/test_capi/test_immortal.py
Outdated
| for non_immortal in non_immortals: | ||
| with self.subTest(non_immortal=non_immortal): | ||
| self.assertFalse(_testcapi.is_immortal(non_immortal)) | ||
|
|
There was a problem hiding this comment.
If yes, add a comment
And ideally an assert to the implementation (as “executable documentation”).
Co-authored-by: Serhiy Storchaka <[email protected]>
Co-authored-by: Petr Viktorin <[email protected]>
This reverts commit 6014587.
Co-authored-by: Serhiy Storchaka <[email protected]>
Lib/test/test_capi/test_immortal.py
Outdated
| with self.subTest(non_immortal=non_immortal): | ||
| self.assertFalse(_testcapi.is_immortal(non_immortal)) | ||
|
|
||
| # CRASHES _testcapi.is_immortal(NULL) |
There was a problem hiding this comment.
is_immortal(None) checks if None is immortal, it doesn't crash.
I suggest to remove this comment. I don't think that it's worth it to bother with testing NULL, PyUnstable_IsImmortal() now starts with assert(op != NULL);. And it's interesting to test if None is immortal or not.
There was a problem hiding this comment.
I suggest to apply my suggestions to be able to test with NULL.
There was a problem hiding this comment.
That would break the test with None. I think as long as we have the assertion there, it's not worth explicitly testing it.
There was a problem hiding this comment.
class TestCAPI(unittest.TestCase):
def test_immortal_builtins(self):
_testcapi.test_immortal_builtins()
def test_immortal_small_ints(self):
_testcapi.test_immortal_small_ints()There is a problem with existing tests: all methods called test_xxx are run automatically by test_capi.test_misc:
vstinner@mona$ ./python -m test test_capi -v -m test_immortal_small_ints
(...)
test_immortal_small_ints (test.test_capi.test_immortal.TestCAPI.test_immortal_small_ints) ... ok
test_immortal_small_ints (test.test_capi.test_misc.Test_testcapi.test_immortal_small_ints) ... ok
(...)
Tests are run twice. Can you fix these tests as part of your PR? Just use a different function name prefix, such as check_xxx.
Modules/_testcapi/object.c
Outdated
| @@ -131,13 +131,22 @@ pyobject_enable_deferred_refcount(PyObject *self, PyObject *obj) | |||
| return PyLong_FromLong(result); | |||
| } | |||
|
|
|||
|
|
|||
| static PyObject * | |||
| is_immortal(PyObject *self, PyObject *op) | |||
There was a problem hiding this comment.
Oh, I didn't know that Modules/_testcapi/immortal.c exists. Maybe put the new function there?
sys._is_immortalfor identifying immortal objects #128510 afterwards.📚 Documentation preview 📚: https://cpython-previews--129182.org.readthedocs.build/en/129182/c-api/object.html#c.PyUnstable_IsImmortal