Skip to content

gh-128509: Add PyUnstable_IsImmortal for finding immortal objects - #129182

Merged
encukou merged 16 commits into
python:mainfrom
ZeroIntensity:unstable-is-immortal
Jan 27, 2025
Merged

gh-128509: Add PyUnstable_IsImmortal for finding immortal objects#129182
encukou merged 16 commits into
python:mainfrom
ZeroIntensity:unstable-is-immortal

Conversation

@ZeroIntensity

@ZeroIntensityZeroIntensity commented Jan 22, 2025

Copy link
Copy Markdown
Member

Comment threadDoc/c-api/object.rst Outdated
Comment threadDoc/c-api/object.rst Outdated
Comment threadDoc/c-api/object.rst Outdated
Comment threadDoc/c-api/object.rst Outdated
Comment threadDoc/c-api/object.rst Outdated
Comment threadModules/_testcapimodule.c Outdated
ZeroIntensityand others added 4 commits January 22, 2025 12:06
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ZeroIntensity

Copy link
Copy Markdown
MemberAuthor

I think @encukou's review would also be helpful.

Comment threadModules/_testcapi/object.c Outdated
static PyObject *
is_immortal(PyObject *self, PyObject *op)
{
return PyBool_FromLong(PyUnstable_IsImmortal(op));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
returnPyBool_FromLong(PyUnstable_IsImmortal(op));
NULLABLE(op)
returnPyLong_FromLong(PyUnstable_IsImmortal(op));

This allows to test with NULL, and check if the result is not 0 or 1.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to crash with NULL.

Comment threadLib/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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does _testcapi.is_immortal(NULL) crash? If yes, add a comment, otherwise add a test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If yes, add a comment

And ideally an assert to the implementation (as “executable documentation”).

Comment threadDoc/c-api/object.rst Outdated
Comment threadLib/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))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If yes, add a comment

And ideally an assert to the implementation (as “executable documentation”).

ZeroIntensityand others added 4 commits January 23, 2025 08:03
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
This reverts commit 6014587.
Comment threadLib/test/test_capi/test_immortal.py Outdated
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Comment threadLib/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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to apply my suggestions to be able to test with NULL.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would break the test with None. I think as long as we have the assertion there, it's not worth explicitly testing it.

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classTestCAPI(unittest.TestCase):
deftest_immortal_builtins(self):
_testcapi.test_immortal_builtins()
deftest_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.

Comment threadModules/_testcapi/object.c Outdated


static PyObject *
is_immortal(PyObject *self, PyObject *op)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I didn't know that Modules/_testcapi/immortal.c exists. Maybe put the new function there?

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ZeroIntensity@vstinner@encukou@serhiy-storchaka