search

ScottishCovidResponse / SCRCIssueTracking

Central issue tracking repository for all repos in the consortium
6 stars 0 forks source link

Remove memory leak in MBP.cc #617

Closed rwj11 closed 4 years ago

rwj11 commented 4 years ago

Memory allocated in MBP.cc at

mbpchain[p] = new MBPCHAIN(data,model,poptree);

is never deallocated. This can be fixed by changing mbpchain to vector<MBPCHAIN> and using emplace_back() to construct members. (A more flexible alternative which also avoids having to have delete on all code paths is to convert the data structure into a vector<unique_ptr<MBPCHAIN>> and construct using make_unique<> to fill it, but that's not necessary here.) While changing this, it might be useful to move these data structures to be private members of a class object rather than file static. Global & file static storage can lead to a variety of issues, e.g. undefined behaviour due to unspecified order of initialization, which I don't think are a problem at present but seem better avoided for the future.

github-actions[bot] commented 4 years ago

Heads up @chrispooley @ianhinder @rwj11 - the "BEEPmbp" label was applied to this issue.

ianhinder commented 4 years ago

Chris, are you happy for robin to make these changes?

chrispooley commented 4 years ago

Hi, I am fine with this change as long as the code still works! Thanks

rwj11 commented 4 years ago

Chris has OK'd this, but it needs to be actioned... The simplest workflow for such issues is probably to reassign them to the relevant developer, rather than close them.

chrispooley commented 4 years ago

OK, thanks! Sorry this entire process of using github is new to me so I make some mistakes...

rwj11 commented 4 years ago

That's fine -- I've used similar systems, but not this one, and we're all getting used to the workflow. I'm learning too...

ianhinder commented 4 years ago

PR is here: https://github.com/ScottishCovidResponse/BEEPmbp/pull/17/

ianhinder commented 4 years ago

This needs the merge to be tidied up a bit and force-pushed over dev.

ianhinder commented 4 years ago

Unfortunately the merge was very complicated and was taking too much time. I've fixed the memory leak in a simple way. It might be good to avoid the new/delete in future, but other things are more urgent.