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-90699: Remove remaining _Py_IDENTIFIER stdlib usage #99067

Merged
merged 14 commits into from
Nov 7, 2022

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented Nov 3, 2022

Automerge-Triggered-By: GH:ericsnowcurrently

Include/internal/pycore_global_strings.h Outdated Show resolved Hide resolved
Modules/_testcapimodule.c Outdated Show resolved Hide resolved
Modules/_datetimemodule.c Show resolved Hide resolved
@bedevere-bot
Copy link

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.

@kumaraditya303 kumaraditya303 marked this pull request as ready for review November 4, 2022 11:20
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

(Thanks for the changes.)

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

One small thing and then LGTM.

Also, were you planning on removing _Py_IDENTIFIER() from Programs/_testembed.c? (Presumably we'd drop test_unicode_id_init().) Then again, that should probably be part of the issue/PR that actually eliminates _Py_IDENTIFIER().

Modules/ossaudiodev.c Show resolved Hide resolved
@bedevere-bot
Copy link

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.

@kumaraditya303
Copy link
Contributor Author

Then again, that should probably be part of the issue/PR that actually eliminates _Py_IDENTIFIER().

Yeah, will remove that in PR which removes _Py_IDENTIFIER together, no plans for that though.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@ericsnowcurrently ericsnowcurrently changed the title GH-90699: Remove remaining _Py_IDENTIFIER usage GH-90699: Remove remaining _Py_IDENTIFIER stdlib usage Nov 7, 2022
@miss-islington miss-islington merged commit be0d500 into python:main Nov 7, 2022
@vstinner
Copy link
Member

vstinner commented Nov 7, 2022

Refleak buildbots fail since this commit. Example:
https://buildbot.python.org/all/#/builders/217/builds/878

2 tests failed:
    test_finalization test_gc

Example:

$ make && ./python -m test -R 3:3 test_finalization -m test_legacy_self_cycle 
...
test_finalization leaked [1, 1, 1] references, sum=3
...

cc @pablogsal

@pablogsal
Copy link
Member

pablogsal commented Nov 7, 2022

Please @ericsnowcurrently @kumaraditya303 take a look, otherwise we will need to revert if is not fixed in 24h per our buildbot policy.

@ericsnowcurrently
Copy link
Member

If @kumaraditya303 doesn't get to it first, I'll tackle this first thing tomorrow morning (~15 hours from now).

@kumaraditya303 kumaraditya303 deleted the rest-pyid branch November 8, 2022 05:23
@kumaraditya303
Copy link
Contributor Author

#99236 fixes the refleak.

@vstinner
Copy link
Member

vstinner commented Nov 8, 2022

The PR got merged, I confirm that it does fix the issue:

$ ./python -m test -R 3:3 test_gc test_finalization
(...)
Tests result: SUCCESS

@ericsnowcurrently
Copy link
Member

Thanks, @kumaraditya303!

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

Successfully merging this pull request may close these issues.

6 participants