Skip to content

bpo-34190: Fix reference leak in call_function()#8413

Merged
vstinner merged 1 commit into
python:masterfrom
jdemeyer:bpo34190
Jul 23, 2018
Merged

bpo-34190: Fix reference leak in call_function()#8413
vstinner merged 1 commit into
python:masterfrom
jdemeyer:bpo34190

Conversation

@jdemeyer

@jdemeyer jdemeyer commented Jul 23, 2018

Copy link
Copy Markdown
Contributor

Needs backport to 3.7

I'm not sure whether this needs a NEWS entry. While the bug existed before #8300, one could argue that this is really just an improvement to #8300.

https://bugs.python.org/issue34190

Comment thread Python/ceval.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hum, I propose to be more explicit here:

}
else {
x = NULL;
}

instead of always initializing x to NULL.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but I proposed a minor coding style change.

@jdemeyer

Copy link
Copy Markdown
Contributor Author

Personally, I think it's better style to initialize x = NULL (it automatically avoids errors where one would forget to assign x) but I'll make the change anyway.

@jdemeyer

Copy link
Copy Markdown
Contributor Author

Don't forget to add the backport to 3.7 label.

@serhiy-storchaka serhiy-storchaka added type-bug An unexpected behavior, bug, or error needs backport to 3.6 labels Jul 23, 2018
@jdemeyer

Copy link
Copy Markdown
Contributor Author

Backport to 3.7, not 3.6 (which doesn't contain the bug yet)

Comment thread Python/ceval.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nitpick: please exchange the two branches, first the common func != NULL case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

first the common func != NULL case.

Where in PEP 7 does it say that the most common case in an if should be first?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is no guide line for that. It's just my preference.

@vstinner

Copy link
Copy Markdown
Member

Personally, I think it's better style to initialize x = NULL (it automatically avoids errors where one would forget to assign x)

The compiler complains if we use x without initializing it. I asked to not initialize x to help the compiler to emit the most efficient code, since call_function() is critical to Python performances.

@jdemeyer

Copy link
Copy Markdown
Contributor Author

I asked to not initialize x to help the compiler to emit the most efficient code, since call_function() is critical to Python performances.

If your compiler is unable to optimize useless assignments, get a better compiler. Seriously, the compiler is generally better at optimizing code than you are.

@vstinner vstinner merged commit 147d955 into python:master Jul 23, 2018
@miss-islington

Copy link
Copy Markdown
Contributor

Thanks @jdemeyer for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 23, 2018
(cherry picked from commit 147d955)

Co-authored-by: jdemeyer <jdemeyer@cage.ugent.be>
@bedevere-bot

Copy link
Copy Markdown

GH-8418 is a backport of this pull request to the 3.7 branch.

vstinner pushed a commit that referenced this pull request Jul 23, 2018
(cherry picked from commit 147d955)

Co-authored-by: jdemeyer <jdemeyer@cage.ugent.be>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip news type-bug An unexpected behavior, bug, or error

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants