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
gh-113688: Split up gcmodule.c #113715
Conversation
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.
Note for reviewers: The PR is split in two commits in an attempt to make it easier to review. The first commit simply moves 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:
|
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
|
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.
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.
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/ |
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
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 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.
@pablogsal - that makes sense. Should I revert the style changes and put them out as a subsequent PR? |
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 |
Great - would one of the reviewers please merge the PR? |
This comment was marked as resolved.
This comment was marked as resolved.
False positive, the builder is out of disk space |
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.