[bindings] Fix some memory leaks in python bindings #2324
Conversation
| Py_BuildValue( "ON", pystatus, Py_BuildValue( "" ) ); | ||
| Py_DECREF( pystatus ); |
There was a problem hiding this comment.
Wouldn't the below do the same, i.e. use "NN" and drop the explicit Py_DECREF( pystatus );?
| Py_BuildValue( "ON", pystatus, Py_BuildValue( "" ) ); | |
| Py_DECREF( pystatus ); | |
| Py_BuildValue( "NN", pystatus, Py_BuildValue( "" ) ); |
There was a problem hiding this comment.
hi. yes; I think that would do the same.
There was a problem hiding this comment.
Ok, I'm fine with the current version too. Please just move the PR out of draft when it's ready for review/merge. Thanks!
There was a problem hiding this comment.
BTW, the macOS errors have nothing to do with this, those tests sporadically fail on macOS, something we need to look at eventually, but for now, just push again or re-run the macOS build by hand and it should pass.
|
hi. right; i'm retrying the macos test. I'll move the PR out of draft; I wasn't able to find any more evident memory leaks, although I feel I may have missed some, given there were a number of places that needed changes, and some I only discovered some via tests that happened to reveal them. There is some repeated initialisation of some python type object(s), e.g. at least variable "URLType", as these are declared static variables in .hh files. When included in different .cc files these refer to different instances of the variable. However I don't think the this actually gives a memory leak (a few too many objects allocated, but number is bounded), or any other problem as far as I could see. The easiest way to fix (given c++17) seemed to be to make these 'inline' rather than static, but for this patch I thought I'd leave that out. Can make a separate PR for that. |
Hi @smithdh, after confirming the memory leaks are fixed, I'm merging this as is. We can change the code with my suggestion later. A patch for the initialization, changing |
No description provided.