Skip to content

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

Merged
merged 1 commit into from
Sep 30, 2020
Merged

BLD: Fixed ARGOUTVIEWM memory deallocation. Closes #17398. #17399

merged 1 commit into from
Sep 30, 2020

Conversation

leakec
Copy link
Contributor

@leakec leakec commented Sep 29, 2020

Closes #17398

Copy link
Member

@eric-wieser eric-wieser left a 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...

@leakec
Copy link
Contributor Author

leakec commented Sep 29, 2020

@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.

@mattip
Copy link
Member

mattip commented Sep 29, 2020

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)

@leakec
Copy link
Contributor Author

leakec commented Sep 29, 2020

@mattip I could add a test similar to Array in numpy/tools/swig/test to the same folder. What do you think?

@mattip
Copy link
Member

mattip commented Sep 30, 2020

I don't think those tests are actually run. You can test that locally by running python runtests.py, it should fail before this PR and pass afterward.

@leakec
Copy link
Contributor Author

leakec commented Sep 30, 2020

@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.

@mattip mattip merged commit 6094508 into numpy:master Sep 30, 2020
@mattip
Copy link
Member

mattip commented Sep 30, 2020

Thanks @leakec

@leakec leakec deleted the argoutviewm_array-fixes branch September 30, 2020 15:48
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.

Potential Issue with 2-D ARGOUTVIEWM Wrapping
3 participants