[fix] PyList_Append is known to memleak #1019

Merged
merged 1 commit into from May 28, 2012

Conversation

Projects
None yet
2 participants
@amet
Contributor

amet commented May 26, 2012

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz May 26, 2012

Member

An alternative fix is to use a temporary:

PyObject *obj = Py_BuildValue(...);
PyList_Append(foo, obj);
Py_DECREF(obj);

Either is fine. The only advantage of this version is you could add error checking (ensuring obj is created successfully) if you wanted to, though I suspect we don't do this anywhere else.

If you stick with your method here (make sure you preallocate the list the right size as in your link), it might pay to add a comment that PyList_SetItem() steals the ref count, so no need to DECREF.

Member

jmarshallnz commented May 26, 2012

An alternative fix is to use a temporary:

PyObject *obj = Py_BuildValue(...);
PyList_Append(foo, obj);
Py_DECREF(obj);

Either is fine. The only advantage of this version is you could add error checking (ensuring obj is created successfully) if you wanted to, though I suspect we don't do this anywhere else.

If you stick with your method here (make sure you preallocate the list the right size as in your link), it might pay to add a comment that PyList_SetItem() steals the ref count, so no need to DECREF.

@amet

This comment has been minimized.

Show comment Hide comment
@amet

amet May 26, 2012

Contributor

I have done it like that in the other PR I did, this looks like less lines of code and looks "cleaner" as well. Had to use temp obj in the other one due to the nature of the code

Contributor

amet commented May 26, 2012

I have done it like that in the other PR I did, this looks like less lines of code and looks "cleaner" as well. Had to use temp obj in the other one due to the nature of the code

@amet

This comment has been minimized.

Show comment Hide comment
@amet

amet May 27, 2012

Contributor

@jmarshallnz fixed as per your comments, let me know if it needs more fixes

Contributor

amet commented May 27, 2012

@jmarshallnz fixed as per your comments, let me know if it needs more fixes

@ghost ghost assigned amet May 27, 2012

@jmarshallnz

This comment has been minimized.

Show comment Hide comment
@jmarshallnz

jmarshallnz May 27, 2012

Member

Looks good - pull it in at your leisure.

Member

jmarshallnz commented May 27, 2012

Looks good - pull it in at your leisure.

amet added a commit that referenced this pull request May 28, 2012

Merge pull request #1019 from amet/PyList_Append_memleak
[fix] PyList_Append is known to memleak

@amet amet merged commit 135b031 into xbmc:master May 28, 2012

tru added a commit to plexinc/plex-home-theater-public that referenced this pull request Feb 25, 2014

Merge pull request #1019 from RasPlex/PR-InliningSetGetProp
[RasPlex][ENH] Inlining GUIListItem Get / Set / Has Properties
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment