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

Use smart pointers instead of raw pointers #96

Closed
wants to merge 1 commit into from

Conversation

charmoniumQ
Copy link
Contributor

  • Before this, VioManager leaked memory, because it would hold pointers that it does not deallocate when it is deallocated.
  • I fixed this by making VioManager hold "smart pointers", so the memory will automatically be deallocated when the VioManager gets deleted (as opposed to writing a destructor and copy-constructor).
    • However, this does make VioManager uncopyable, but I believe that before this, copies of VioManager were logically incorrect, because the old code would make shallow copies of the internal member pointers.
  • Furthermore, TrackBase needs to have a virtual destructor to deallocate properly.

@goldbattle goldbattle added the enhancement New feature or request label Aug 28, 2020
@goldbattle
Copy link
Member

This looks good, thanks for the PR. I had been meaning to add the de-constructor, but since we never de-allocated until the program ended I never put it at high priority. Using unique_ptrs should also address this issue a bit more elegantly. When I find time I will test and merge, thanks!

@goldbattle
Copy link
Member

I have started working on merging this. I have ended up switching over the shared_ptr to match how I original implemented everything. Feedback welcomed, but I am replacing the majority of all points in the database to this. After changing memcheck doesn't report any "definitely lost" memory losses, so that is a positive as compared to before.

@goldbattle
Copy link
Member

Should be addressed in #117, thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants