Skip to content

Conversation

@efimov-mikhail
Copy link
Member

@efimov-mikhail efimov-mikhail commented Oct 7, 2025

New test file Lib/test/test_stackrefs.py introduced.
Some auxillary functions are added to _testinternalcapi.
Those functions are examples of correct StackRef scenarios.
We check that all refcounts remain correct.
And that results are the same for equivalent scenarios.

@efimov-mikhail
Copy link
Member Author

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

Looks good. I've one concern about code organization, but that's all.

}

static PyObject *
stackref_to_tuple(_PyStackRef ref, PyObject *op)
Copy link
Member

Choose a reason for hiding this comment

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

I worry that stackref_to_tuple is likely to get out of sync when the stackref implementation changes. Can you move it to Python/stackrefs.c, rename it to _PyStackRef_AsTuple and a declaration to pycore_stackref.h

The other functions below are fine here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've done this change. But I can see no much value in moving this function to Python/stackrefs.c, so I create 3 different implementations of this little function at pycore_stackref.h.

@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@efimov-mikhail
Copy link
Member Author

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Oct 27, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon October 27, 2025 16:24
@efimov-mikhail efimov-mikhail marked this pull request as draft October 27, 2025 19:05
@efimov-mikhail efimov-mikhail marked this pull request as ready for review October 27, 2025 19:05
Copy link
Contributor

@sergey-miryanov sergey-miryanov left a comment

Choose a reason for hiding this comment

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

Thanks!

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

Labels

awaiting review skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants