The Wayback Machine - https://web.archive.org/web/20240111222513/https://github.com/python/cpython/pull/113715
Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-113688: Split up gcmodule.c #113715

Merged
merged 5 commits into from Jan 5, 2024
Merged

gh-113688: Split up gcmodule.c #113715

merged 5 commits into from Jan 5, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented Jan 4, 2024

This splits part of Modules/gcmodule.c of into Python/gc.c, which now contains the core garbage collection implementation. The Python module remain in the Modules/gcmodule.c file.

@colesbury colesbury added skip news 3.13 new features, bugs and security fixes labels Jan 4, 2024
This splits part of Modules/gcmodule.c of into Python/gc.c, which
now contains the core garbage collection implementation. The Python
module remain in the Modules/gcmodule.c file.
@colesbury colesbury marked this pull request as ready for review January 4, 2024 21:20
@colesbury colesbury requested review from a team and pablogsal as code owners January 4, 2024 21:20
@colesbury
Copy link
Contributor Author

Note for reviewers: The PR is split in two commits in an attempt to make it easier to review. The first commit simply moves Modules/gcmodule.c to Python/gc.c without changes. The second commit pulls out the Python package back into Modules/gcmodule.c and fixes things up.

I recommend reviewing the second commit: fa068b5 "Split up gcmodule.c".

Here's the summary of changes beyond simply moving functions from one file to another:

  • Move logic for freezing, unfreezing, getting all GC objects, etc. to Python/gc.c. See changes to pycore_gc.h. This avoids requiring many of the GC list operations in Modules/gcmodule.c.
  • Move the DEBUG_STATS, etc. constants to pycore_gc.h and prefix them as e.g., _PyGC_DEBUG_STATS. The Python constants are not renamed (i.e., gc.DEBUG_STATS). They are used in both Modules/gcmodule.c and Python/gc.c.

@nascheme
Copy link
Member

nascheme commented Jan 4, 2024

Looks okay to me. I'd suggest it's time to revise that comment at the top of the file. It's not so useful anymore and someone can use git log to see contributors and the history. Maybe add a link to https://devguide.python.org/internals/garbage-collector/ instead of links to those mailing list posts. E.g. you could put this at the top of the gc.c file instead:

//  This implements the reference cycle garbage collector.
//  The Python module inteface to the collector is in gcmodule.c.
//  See https://devguide.python.org/internals/garbage-collector/

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Thanks, this is a nice refactor; the resulting extension module is a lot easier to read now.

Nitpick: since we're touching these lines, we now have a golden opportunity to align the new Python/gc.c file with PEP-7, so I suggest we do so.

Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
Python/gc.c Outdated Show resolved Hide resolved
@pablogsal
Copy link
Member

pablogsal commented Jan 5, 2024

Looks okay to me. I'd suggest it's time to revise that comment at the top of the file. It's not so useful anymore and someone can use git log to see contributors and the history. Maybe add a link to https://devguide.python.org/internals/garbage-collector/ instead of links to those mailing list posts. E.g. you could put this at the top of the gc.c file instead:

//  This implements the reference cycle garbage collector.
//  The Python module inteface to the collector is in gcmodule.c.
//  See https://devguide.python.org/internals/garbage-collector/

Related to this although not a blocker for this PR: let's ensure we document the nogil parts also in https://devguide.python.org/internals/garbage-collector/

colesbury and others added 2 commits January 5, 2024 11:23
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

I made a first round of review and everything looks good to me.

I would advise to keep this PR at minimum changes (so let's avoid the temptation to do refactors 😉 ) to keep the split as "pure" as possible.

@colesbury
Copy link
Contributor Author

@pablogsal - that makes sense. Should I revert the style changes and put them out as a subsequent PR?

@pablogsal
Copy link
Member

Nah, don't worry, I agree it was a good opportunity but I was being careful that we end putting a lot more of other stylistic changes or similar

@colesbury
Copy link
Contributor Author

Great - would one of the reviewers please merge the PR?

@nascheme nascheme merged commit 99854ce into python:main Jan 5, 2024
30 checks passed
@bedevere-bot

This comment was marked as resolved.

@pablogsal
Copy link
Member

False positive, the builder is out of disk space

@colesbury colesbury deleted the gh-113688 branch January 5, 2024 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.13 new features, bugs and security fixes skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants