-
-
Notifications
You must be signed in to change notification settings - Fork 10.8k
BLD: Fixed ARGOUTVIEWM memory deallocation. Closes #17398. #17399
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch looks good.
It's a little alarming how many of these there are. It's almost tempting just to remove all the broken ones, since I imagine no one has used them in a decade, and the other argument order works fine...
@eric-wieser If its useful, I ran into the error because I needed to use the (int* DIM1, int* DIM2, double** ARGOUTVIEWM_ARRAY2) typemap in a project: the issue I originally submitted is a simple working example based on that project. If this typemap were removed, I would need to reformat quite a few of the function declarations in the project. However, I'm not sure if this is a common occurrence. If no one has submitted the issue for a decade, I suppose its used very rarely. Another thought: if all the versions of the ARGOUTVIEW typemap are used commonly, users may be interested in changing to ARGOUTVIEWM if they run into memory leak issues (that was the case for me). If all of the versions of the ARGOUTVIEWM typemaps are available, it is very easy for the user to change their ARGOUTVIEW typemap to a ARGOUTVIEWM typemap by simply adding a letter to their .i file. |
It would be nice to have tests for swig. We have tests for the random C-API using example programs in random, we have tests for f2py. Could we add some tests based off of those examples? If it is difficult, we should merge this PR as a band-aid and encourage some capable swig people to add new tests (hint, hint) |
@mattip I could add a test similar to Array in numpy/tools/swig/test to the same folder. What do you think? |
I don't think those tests are actually run. You can test that locally by running |
@mattip Yeah you're right. I checked the build log, and runtests never even enters the swig directory. To create these tests there would obviously need to be a call to swig in order to generate the wrapper C code needed to compile into a Python package. I'm not familiar with NumPy's build system, and unfortunately don't feel I have the time to dedicate at the moment to become familiar enough to learn it (currently in the dissertation writing phase of my PhD, so I'm pretty busy at the moment). Therefore, I'd like to go with your original suggestion and merge this PR as a band-aid and encourage some capable swig people to add new tests. Please let me know if there is anything I need to do on my end to make this happen. |
Thanks @leakec |
Closes #17398