Skip to content

bpo-30547: Fix multiple reference leaks #1995

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

Merged
merged 1 commit into from
Jun 8, 2017
Merged

Conversation

matrixise
Copy link
Member

@matrixise matrixise commented Jun 8, 2017

Fix regressions introduced by:

Fixed with @Haypo and @mlouielu

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

In the commit message, can you please use the syntax:

Co-Authored-By: full name <[email protected]>

Rather than GitHub names? For me, it's:

Co-Authored-By: Victor Stinner <[email protected]>

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Commit message:
"Introduced by 1abcf67"

Hum, it was also introduced by 6b4be19. I propose:

Fix regressions introduced by:

- bpo-18520: commit 8fea252a507024edf00d5d98881d22dc8799a8d3
- bpo-22257: commits 1abcf6700b4da6207fe859de40c6c1bada6b4fec and 6b4be195cd8868b76eb6fbe166acc39beee8ce36

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, let's see if all tests pass ;-)

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I suggest you to revert the warnoptions change and let it open, since it seems tricky to fix it.

@@ -2139,7 +2139,7 @@ _PySys_EndInit(PyObject *sysdict)
else {
Py_INCREF(warnoptions);
}
SET_SYS_FROM_STRING_BORROW_INT_RESULT("warnoptions", warnoptions);
SET_SYS_FROM_STRING_INT_RESULT("warnoptions", warnoptions);
Copy link
Member

Choose a reason for hiding this comment

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

test_capi does crash with this change :-(

Copy link
Member Author

Choose a reason for hiding this comment

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

ok thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

reverted

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

You reverted the warnoptions change, but you forgot to update the commit message: your change is not related to "- bpo-18520: commit 8fea252".

Fix regressions introduced by:

- bpo-22257: commits 1abcf67 and 6b4be19

Co-Authored-By: Victor Stinner <[email protected]>
Co-Authored-By: Louie Lu <[email protected]>
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Ok, it should now pass on Travis CI but it doesn't fix the refleak in warnoptions. It's a compromise :-)

@vstinner vstinner merged commit ab1cb80 into python:master Jun 8, 2017
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