Where/how to delete an object within another objec

Question

In order to solve this problem Bad memory management? Class member (boolean) value greater than 1, in recursion function, I ran the whole program under Valgrind and found a few memory leak problems that occurred before the step. There were 2 'definitely lost' problems identified in the function CMFLoader.

(here MyDataset is a vector of Molecule objects, and each Molecule object contains an Elements object)

In CMFLoader::loadFile(vector& MyDataset), I originally have

MyDataset.push_back( readMolecule( in_file, word );

where CMFLoader::readMolecule returns a Molecule object. Within the readMolecule function, a new Molecule object is created (but not deleted until the very end of main, more on that later)

Molecule* CMFLoader::readMolecule( ifstream& in_file, string id)
{
Molecule* my_mol = new Molecule( id );
// statements, somewhere along the readFormula function is called
my_mol->setFormula( readFormula( ss ) );
return my_mol;
}

where CMFLoader::readFormula returns an Element object and there is a Molecule::setFormula function to save it to the Molecule object. In readFormula

Elements* CMFLoader::readFormula( stringstream& ss )
{
Elements* my_formula = new Elements();
...
return my_formula;
}

I ran into problems described in the question here, later in the main program. The specific problem occurred at the HammettCheck::checkHammett step. I then changed the above CMFLoader functions to something like this. The problems I encountered earlier seems to have disappeared (but there were other problems later in the program that were no doubt related to the memory leak):

in CMFLoader::loadFile

Molecule* new_mol = new Molecule(word);
    MyDataset.push_back( readMolecule( in_file, word ,new_mol) );

where readMolecule now takes in a new argument Molecule* and the new operator is removed within the function. Similarly, in readFormula, I now have

 Elements* new_formula = new Elements();
my_mol->setFormula( readFormula( ss, new_formula ) );

etc.

Now of course the memory leak problem is not removed! However, I cannot put in the delete operator within any of the CMFLoader functions as the objects are used later in the main program. Specifically, Elements* is used till the ConjugationCheck::checkConjugation step, and Molecule* is used till the program ends.

Main program goes like this

int main(int argc, char* argv[]){
//initialising an empty array to store our molecules.
vector<Molecule*> MyDataset;

//Read command line inputs.
InputReader* MyInputs = new InputReader();
if( !MyInputs->readInputs(argc, argv) ) {delete MyInputs;return -1;}

//Load CMF file.
CMFLoader* MyLoader = new CMFLoader( MyInputs );
unsigned int min_same_grp = MyLoader->getmin(); //define minimum no of same hammett groups for structure
if( !MyLoader->loadFile( MyDataset ) ) {delete MyLoader;delete MyInputs;return -1;}
delete MyLoader;

cout << MyDataset.size() << " molecules loaded" << endl;

//Remove molecules which are too large.
BigFilter* MyBigFilter = new BigFilter( MyInputs );
if( !MyBigFilter->filterBigLigands( MyDataset ) ) {delete MyBigFilter;delete MyInputs;return -1;}
delete MyBigFilter;

cout << "Molecules left after big ligand filter: " << MyDataset.size() << endl;

//Mark any Hammetts groups found in molecules.
HammettCheck* MyHammettCheck = new HammettCheck(min_same_grp);
if( !MyHammettCheck->loadHammetts() ) {delete MyHammettCheck;delete MyInputs;return -1;}
if( !MyHammettCheck->checkHammett( MyDataset ) ) {delete MyHammettCheck;delete MyInputs;return -1;}
delete MyHammettCheck;

cout << "Molecules containing Hammett Groups: " << MyDataset.size() << endl;

ConjugationCheck* MyConjugationCheck = new ConjugationCheck(min_same_grp);
if( !MyConjugationCheck->checkConjugation( MyDataset ) ) {delete MyConjugationCheck;delete MyInputs;return -1;}
delete MyConjugationCheck;

cout << "Molecules containing conjugated Hammett Groups: " << MyDataset.size() << endl;

DataAdder* MyDataAdder = new DataAdder( MyInputs );
if( !MyDataAdder->addData( MyDataset ) ) {delete MyDataAdder; delete MyInputs;return -1;}
delete MyDataAdder;

//Sorts molecules based on their NLO rating given by NLOCompare.
if (min_same_grp ==1) {sort(MyDataset.begin(), MyDataset.end(), NLOCompare);}
else {sort(MyDataset.begin(), MyDataset.end(), OctuNLOCompare);}

//Saves a new CIF file containing just the predicted NLO molecules.
FileSaver* MyFileSaver = new FileSaver( MyInputs );
if( !MyFileSaver->saveFile( MyDataset ) ) {delete MyFileSaver;delete MyInputs;return -1;}
delete MyFileSaver;

/*
Saves a txt file which can be imported into Excel, showing the
paths to each of the selected Hammett groups in a molecule.
*/
ExcelSaver* MyExcelSaver = new ExcelSaver( MyInputs );
if( !MyExcelSaver->saveFile( MyDataset ) ) {delete MyExcelSaver;delete MyInputs;return -1;}
delete MyExcelSaver;

//Cleans the memory before exiting the program.
for(unsigned int i=0; i < MyDataset.size(); i++){
    delete MyDataset[i];
}
delete MyInputs;
return 0;
}

At various points in the program, if the Molecule MyDataset[i] does not fit certain conditions, it is removed using

MyDataset.pop_back();

So this would call the Molecule Destructor, which looks like this

Molecule::~Molecule(void)
{
//Deletes all atoms in molecule.
for(unsigned int i=0; i < mol_atoms.size(); i++){
    delete mol_atoms[i];
}

//Deletes all bonds in molecule.
for(unsigned int i=0; i < mol_bonds.size(); i++){
    delete mol_bonds[i];
}

//Deletes the class of elements contained.
delete mol_formula;
}

I am not sure what went wrong here. How should I fix the memory leak problem?

The "definitely loss" problems in my Valgrind Memcheck Leak summary

==34809== 400 (96 direct, 304 indirect) bytes in 2 blocks are definitely lost in loss record 24 of 33
==34809==    at 0x1000A0679: malloc (vg_replace_malloc.c:266)
==34809==    by 0x1000F7F04: operator new(unsigned long) (in /usr/lib/libstdc++.6.0.9.dylib)
==34809==    by 0x10000A3B4: CMFLoader::readMolecule(std::basic_ifstream<char, std::char_traits<char> >&, std::string, Molecule*) (in ./OctuDiscovery)
==34809==    by 0x10000B9EE: CMFLoader::loadFile(std::vector<Molecule*, std::allocator<Molecule*> >&) (in ./OctuDiscovery)
==34809==    by 0x10000282E: main (in ./OctuDiscovery)

==34809== 12,833 (152 direct, 12,681 indirect) bytes in 1 blocks are definitely lost in loss record 33 of 33
==34809==    at 0x1000A0679: malloc (vg_replace_malloc.c:266)
==34809==    by 0x1000F7F04: operator new(unsigned long) (in /usr/lib/libstdc++.6.0.9.dylib)
==34809==    by 0x10000B93B: CMFLoader::loadFile(std::vector<Molecule*, std::allocator<Molecule*> >&) (in ./OctuDiscovery)
==34809==    by 0x10000282E: main (in ./OctuDiscovery)

Answer 1

More a comment than an answer, but too long for a comment:
In next function, it doesn't make sense to use dynamic memory:

Molecule* CMFLoader::readMolecule( ifstream& in_file, string id)
{
Molecule* my_mol = new Molecule( id );
// statements, somewhere along the readFormula function is called
my_mol->setFormula( readFormula( ss ) );
return my_mol;
}

You could replace it with:

Molecule CMFLoader::readMolecule( ifstream& in_file, string id)
{
Molecule my_mol( id );
// statements, somewhere along the readFormula function is called
my_mol.setFormula( readFormula( ss ) );
return my_mol;
}

This solves already 1 possible memory leak, but there might be reasons why the dynamic memory version is needed/preferred, in which case the already mentioned unique_ptr should be used.

Answer 2

If you want to do something like this, I would suggest that you take a look at std::auto_ptr class, or at least the concepts of smart pointers, or auto pointers. You should rely on RAII (Resource Aquisition Is Initialization) paradigm, meaning that the memory management should be done by objects themselves: avoid basic pointers and self written memory management code whenever you can.