Skip to content
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

fixed several memory leaks of local HD5Read buffers #271

Merged
merged 1 commit into from Jan 25, 2014
Merged

fixed several memory leaks of local HD5Read buffers #271

merged 1 commit into from Jan 25, 2014

Conversation

hainest
Copy link

@hainest hainest commented Jan 25, 2014

Several local buffers used in calls to HD5Read were not deallocated before exiting the local scope. Most of these were added by commit a48a540.

@scopatz scopatz merged commit 9cd54c7 into pyne:staging Jan 25, 2014
@scopatz
Copy link
Member

scopatz commented Jan 25, 2014

Thanks for noticing our shortcomings, Tim. Double thanks for the fix.

@crbates
Copy link
Contributor

crbates commented Jan 26, 2014

So in my quest to learn more c++ I recently stumbled across std::unique_ptr which is a nice way in c++11 to not have to worry about deletes. I'm not saying we should use it instead but I figured I would mention it here for future reference.

@hainest
Copy link
Author

hainest commented Jan 26, 2014

@scopatz It was minor stuff, but you are quite welcome. One other thing I noticed in cpp/data.cpp is the usage of empty catch blocks to "pass" the failure of library loads. I am assuming this hasn't caused any issues, but may warrant investigation.

@crbates Indeed. std::unique_ptr can now contain array pointers which std::auto_ptr could not. There is lots of great new stuff in C++11.

@scopatz
Copy link
Member

scopatz commented Jan 26, 2014

@scopatz It was minor stuff, but you are quite welcome. One other thing I noticed in cpp/data.cpp is the usage of empty catch blocks to "pass" the failure of library loads. I am assuming this hasn't caused any issues, but may warrant investigation.

This is because these functions have model fallbacks (or should) if the data is not available. Moreover we can't guarantee that the data will be available on any given machine. Such differences should show up in the tests, though.

So in my quest to learn more c++ I recently stumbled across std::unique_ptr which is a nice way in c++11 to not have to worry about deletes. I'm not saying we should use it instead but I figured I would mention it here for future reference.

@crbates Indeed. std::unique_ptr can now contain array pointers which std::auto_ptr could not. There is lots of great new stuff in C++11.

My personal opinion about this is that as a rule we shouldn't be using C/C++ smart pointers. If you want that kind of memory management in PyNE, just use Python or Cython (with the public flag). The thing that differentiates C/C++ is that they don't force you to have smart pointers.

Also, I am not convinced that C++11 is really ready for the prime time yet (much like Python 3), sadly. There are things that I would love to get rid of first (like std::map), etc.

@crbates crbates mentioned this pull request Jan 26, 2014
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.

None yet

3 participants