Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-128509: AddPyUnstable_IsImmortal for finding immortal objects#129182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM.
I think@encukou's review would also be helpful. |
Modules/_testcapi/object.c Outdated
| staticPyObject* | ||
| is_immortal(PyObject*self,PyObject*op) | ||
| { | ||
| returnPyBool_FromLong(PyUnstable_IsImmortal(op)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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.
There was a problem hiding this comment.
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 withNULL.
Lib/test/test_capi/test_immortal.py Outdated
| fornon_immortalinnon_immortals: | ||
| withself.subTest(non_immortal=non_immortal): | ||
| self.assertFalse(_testcapi.is_immortal(non_immortal)) | ||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 anassert to the implementation (as “executable documentation”).
Uh oh!
There was an error while loading.Please reload this page.
Lib/test/test_capi/test_immortal.py Outdated
| fornon_immortalinnon_immortals: | ||
| withself.subTest(non_immortal=non_immortal): | ||
| self.assertFalse(_testcapi.is_immortal(non_immortal)) | ||
There was a problem hiding this comment.
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 anassert to the implementation (as “executable documentation”).
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Co-authored-by: Petr Viktorin <encukou@gmail.com>
This reverts commit6014587.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
Lib/test/test_capi/test_immortal.py Outdated
| withself.subTest(non_immortal=non_immortal): | ||
| self.assertFalse(_testcapi.is_immortal(non_immortal)) | ||
| # CRASHES _testcapi.is_immortal(NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Did you test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
is_immortal(None) checks ifNone 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 withassert(op != NULL);. And it's interesting to test if None is immortal or not.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 withNone. I think as long as we have the assertion there, it's not worth explicitly testing it.
There was a problem hiding this comment.
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 calledtest_xxx are run automatically bytest_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) ... oktest_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 ascheck_xxx.
Modules/_testcapi/object.c Outdated
| staticPyObject* | ||
| is_immortal(PyObject*self,PyObject*op) |
There was a problem hiding this comment.
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 thatModules/_testcapi/immortal.c exists. Maybe put the new function there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
3fb5f6e intopython:mainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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