GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-128509: Add `PyUnstable_IsImmortal` for finding immortal objects by ZeroIntensity · Pull Request #129182 · python/cpython

Conversation

ZeroIntensity commented Jan 22, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

I've reused the test from my other PR.
The plan is to go for this, and then possibly go for gh-128509: Add sys._is_immortal for identifying immortal objects #128510 afterwards.

Issue: Add an API for determining if an object is immortal #128509

📚 Documentation preview 📚: https://cpython-previews--129182.org.readthedocs.build/en/129182/c-api/object.html#c.PyUnstable_IsImmortal

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ZeroIntensity added 5 commits

January 22, 2025 09:20


          Add PyUnstable_IsImmortal

58ac265


          Add a test.

8a64537


          Fix test.

4b55134


          Add a whatsnew entry.

6bddf33


          Add blurb

412a42b

ZeroIntensity requested a review from vstinner

January 22, 2025 14:30

bedevere-app Bot added the awaiting review label

Jan 22, 2025

bedevere-app Bot mentioned this pull request

Jan 22, 2025

Add an API for determining if an object is immortal #128509

Closed

vstinner reviewed

Jan 22, 2025

View reviewed changes

Comment thread

Doc/c-api/object.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Doc/c-api/object.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Doc/c-api/object.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Doc/c-api/object.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Doc/c-api/object.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_testcapimodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity and others added 4 commits

January 22, 2025 12:06


          Update object.rst

21cf2c4

Co-authored-by: Victor Stinner <[email protected]>


          Update object.rst

8d20c00

Co-authored-by: Victor Stinner <[email protected]>


          Remove some information.

831626c


          Move test function.

422edf9

ZeroIntensity requested a review from vstinner

January 22, 2025 21:16

vstinner approved these changes

Jan 22, 2025

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

cc @colesbury @serhiy-storchaka

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

colesbury reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-app Bot added awaiting merge and removed awaiting review labels

Jan 22, 2025

ZeroIntensity commented Jan 22, 2025

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Jan 23, 2025

View reviewed changes

Comment thread

Modules/_testcapi/object.c Outdated

		static PyObject *
		is_immortal(PyObject self, PyObject op)
		{
		return PyBool_FromLong(PyUnstable_IsImmortal(op));

serhiy-storchaka Jan 23, 2025

Copy link

Copy Markdown

Member

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.

Suggested change

	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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ZeroIntensity Jan 23, 2025

Copy link

Copy Markdown

Member Author

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 with NULL.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

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))

serhiy-storchaka Jan 23, 2025

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

encukou Jan 23, 2025

Copy link

Copy Markdown

Member

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 an assert to the implementation (as “executable documentation”).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity reacted with thumbs up emoji

All reactions

👍 1 reaction

encukou reviewed

Jan 23, 2025

View reviewed changes

Comment thread

Doc/c-api/object.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

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))

encukou Jan 23, 2025

Copy link

Copy Markdown

Member

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 an assert to the implementation (as “executable documentation”).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity reacted with thumbs up emoji

All reactions

👍 1 reaction

ZeroIntensity and others added 4 commits

January 23, 2025 08:03


          Update object.c

Co-authored-by: Serhiy Storchaka <[email protected]>


          Update object.rst

48c974c

Co-authored-by: Petr Viktorin <[email protected]>


          Add non-null assertion

dd19a02


          Revert "Update object.c"

99e897f

This reverts commit 6014587.

serhiy-storchaka reviewed

Jan 23, 2025

View reviewed changes

Comment thread

Lib/test/test_capi/test_immortal.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Lib/test/test_capi/test_immortal.py

0448daf

Co-authored-by: Serhiy Storchaka <[email protected]>

serhiy-storchaka reviewed

Jan 24, 2025

View reviewed changes

Comment thread

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)

serhiy-storchaka Jan 24, 2025

Copy link

Copy Markdown

Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Jan 24, 2025

Copy link

Copy Markdown

Member

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jan 24, 2025

Copy link

Copy Markdown

Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ZeroIntensity Jan 24, 2025

Copy link

Copy Markdown

Member Author

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 with None. I think as long as we have the assertion there, it's not worth explicitly testing it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner reviewed

Jan 24, 2025

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity reacted with thumbs up emoji

All reactions

👍 1 reaction

vstinner reviewed

Jan 24, 2025

View reviewed changes

Comment thread

Modules/_testcapi/object.c Outdated



		static PyObject *
		is_immortal(PyObject self, PyObject op)

vstinner Jan 24, 2025

Copy link

Copy Markdown

Member

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 that Modules/_testcapi/immortal.c exists. Maybe put the new function there?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ZeroIntensity reacted with thumbs up emoji

All reactions

👍 1 reaction


          Prevent tests from running twice and move is_immortal()

5b949b1


          Fix missing comma.

d1c9b1f

ZeroIntensity requested review from encukou and serhiy-storchaka

January 24, 2025 13:29

vstinner approved these changes

Jan 24, 2025

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

encukou merged commit 3fb5f6e into python:main

Jan 27, 2025

bedevere-app Bot removed the awaiting merge label

Jan 27, 2025

ZeroIntensity mentioned this pull request

Jan 27, 2025

gh-128509: Add sys._is_immortal for identifying immortal objects #128510

Merged

ZeroIntensity deleted the unstable-is-immortal branch

January 27, 2025 15:42

Reviewers

vstinner

vstinner approved these changes

encukou

Awaiting requested review from encukou

serhiy-storchaka

Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

ZeroIntensity commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity commented Jan 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ZeroIntensity commented Jan 22, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.