Skip to content

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Oct 10, 2024

Modified PyInitConfig_Free, to release the memory occupied when the config argument isn't NULL.

PyInitConfig *config = NULL;
// some code
if (error_occurred) {
    goto Error;
}
config = PyInitConfig_Create();
if (config == NULL) {
    goto Error;
}
// some other code
if (error_occurred) {
    goto Error;
}
// more code
Error:
// release resources
PyInitConfig_Free(config);
// release resources

(I hope my understanding is correct).

@bedevere-app
Copy link

bedevere-app bot commented Oct 10, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app
Copy link

bedevere-app bot commented Oct 10, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@rruuaanng rruuaanng marked this pull request as ready for review October 10, 2024 14:31
@bedevere-app
Copy link

bedevere-app bot commented Oct 10, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

LGTM

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Oct 10, 2024

This implementation looks fine, but the point of the original issue was to simplify things. Should that be done with this PR as well? (My approval was for the implementation, but Serhiy probably wants to deal with the use of it internally.)

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Since this is a public C API change, it should be documented. Add a versionchanged directive and the What's New entry.

I am going also to get an approve of the C API Workgroup.

@ZeroIntensity
Copy link
Member

This is already in "What's New" under PEP 741, I think.

@serhiy-storchaka
Copy link
Member

Ah, it is new in 3.14. Then nothing of this is needed.

@ZeroIntensity
Copy link
Member

Yeah. In fact, do we even need a NEWS entry for changes to unreleased features?

@serhiy-storchaka
Copy link
Member

A NEWS entry is not needed.

@rruuaanng
Copy link
Contributor Author

A NEWS entry is not needed

Maybe I can remove it.

@bedevere-app
Copy link

bedevere-app bot commented Oct 11, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@rruuaanng
Copy link
Contributor Author

Maybe, I have made the corresponding changes (I hope).

@rruuaanng

This comment was marked as off-topic.

@ZeroIntensity
Copy link
Member

You didn't trigger the bot. I think saying I didn't expect the Spanish Inquisition should work too 😉

@rruuaanng
Copy link
Contributor Author

Oh, This is really interesting! Hope this works🙂

@ZeroIntensity
Copy link
Member

(You have to say it, the bot ignores comments from people other than the author.)

@rruuaanng
Copy link
Contributor Author

I didn't expect the Spanish Inquisition

@bedevere-app
Copy link

bedevere-app bot commented Oct 15, 2024

Nobody expects the Spanish Inquisition!

@erlend-aasland, @kumaraditya303: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from kumaraditya303 October 15, 2024 01:26
@rruuaanng

This comment was marked as off-topic.

@erlend-aasland
Copy link
Contributor

@rruuaanng: every time you make a comment on an issue or a PR, all subscribed parties receives a notification, or even an email. Please be respectful of others time and don't use GitHub as a forum. We have Discourse for that functionality.

@erlend-aasland erlend-aasland changed the title gh-125234: Integrated NULL check in PyInitConfig_Free() gh-125234: Make PyInitConfig_Free(NULL) a no-op Oct 15, 2024
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.

Please also update the docs. Look to PyMem_Free and PyMem_RawFree for inspiration.

@bedevere-app
Copy link

bedevere-app bot commented Oct 15, 2024

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.

@erlend-aasland erlend-aasland enabled auto-merge (squash) October 15, 2024 09:21
@erlend-aasland erlend-aasland merged commit 546dddc into python:main Oct 15, 2024
38 of 39 checks passed
@ZeroIntensity
Copy link
Member

every time you make a comment on an issue or a PR, all subscribed parties receives a notification, or even an email. Please be respectful of others time and don't use GitHub as a forum.

Yeah, I was a little guilty of this too in this case, sorry :(

In general, what's the procedure for walking someone through the PR process if they're having difficulties?

@rruuaanng rruuaanng deleted the gh125234 branch October 15, 2024 10:53
@erlend-aasland
Copy link
Contributor

Yeah, I was a little guilty of this too in this case, sorry :(

In general, what's the procedure for walking someone through the PR process if they're having difficulties?

No worries, you're both fine. If I notice such a pattern on a PR, I explicitly make a remark and remind the participants of this effect. Using reactions, like thumbs-up, is often sufficient. Also, thinking twice about how often we demand the attention of others may be helpful.

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.

5 participants