Skip to content

gh-124127: Make Py_REFCNT() opaque in limited C API 3.14#124128

Merged
vstinner merged 1 commit into
python:mainfrom
vstinner:refcnt_opaque
Sep 24, 2024
Merged

gh-124127: Make Py_REFCNT() opaque in limited C API 3.14#124128
vstinner merged 1 commit into
python:mainfrom
vstinner:refcnt_opaque

Conversation

@vstinner

@vstinner vstinner commented Sep 16, 2024

Copy link
Copy Markdown
Member

@vstinner

Copy link
Copy Markdown
Member Author

@vstinner

Copy link
Copy Markdown
Member Author

cc @serhiy-storchaka

@vstinner

Copy link
Copy Markdown
Member Author

I plan to merge this change at the end of the week, unless someone wants to review it or has concerns about it.

@serhiy-storchaka serhiy-storchaka 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.

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

+1, with a nitpick on the implementation:

Comment thread Include/refcount.h
}
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000
# define Py_REFCNT(ob) Py_REFCNT(_PyObject_CAST(ob))
#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 < 0x030b0000

@encukou encukou Sep 23, 2024

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.

This #if is now redundant, right?

@vstinner vstinner Sep 24, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's still needed. (Correct me if I'm wrong.)

@vstinner vstinner merged commit 9d344fa into python:main Sep 24, 2024
@vstinner vstinner deleted the refcnt_opaque branch September 24, 2024 06:43
@vstinner

Copy link
Copy Markdown
Member Author

Merged, thanks for reviews.

Sign up for free to 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.

3 participants