CODEWITHSUNDEEP
c++
Jack Nickolson
Jack Nickolson

Reputation: 225

Self destruction of class object

I'm having a hard time freeing memory allocated this way

Text * text = new Text();

I have the iterator

 for(iterator = textList.begin() ; iterator != textList.end() ; ++iterator)
{
    if((*iterator)->getTitle() == element.toStdString())
    {
        textList.remove((*iterator));
        break;
    }
}

textList contains pointers to class objects

    list<Text *>textList;

And destructor 

~Text() {delete this;}

I've read that remove() method actually calls destructor for the object, but for some reason it's not the case. Valgrind clearly shows memory leak. So, I would really appreciate it if you could give me a hint on how I can free that memory.

Upvotes: 0

Views: 2616

Answers (4)

xiaofeng.li
xiaofeng.li

Reputation: 8587

I've read that remove() method actually calls destructor for the object, but for some reason it's not the case.

Because you are storing pointers in the list, not objects. And the container certainly will not try to call delete for you. Why? Because it has no way of knowing whether the object is created by new or not. This is perfectly legal:

list<Text*> textList;

Text text;

textList.push_back(&text);
// You can safely use textList as long as text is in scope.
...

And deleting this in the destructor is simply wrong. If the object is on the stack, you don't need to delete it. If the object is created by new, well like @Pavel said, somewhere in your code the pointer is already deleted. Either way your program crashes.

And you should avoid using new and delete when writing modern C++ code. Do watch this talk by Herb Sutter. And to summarize: Leak Freedom in C++

Upvotes: 0

Pavan Chandaka
Pavan Chandaka

Reputation: 12751

Comments and answers to your question has some good suggestions.

But there is a way I read in scott meyers book for effective STL, exactly for your case.

The comments in below code gives enough explanation.

//g++  5.4.0
#include <algorithm>
#include <iostream>
#include <list>


//A SAMPLE TEXT CLASS TO TEST
class Text
{
public:
    Text()
    {
        std::cout<<"constructor"<<std::endl;
    }

    ~Text()
    {

        std::cout<<"destructor"<<std::endl;
    }

    std::string getTitle()
    {
        return "test";
    }
};



//DECLARE A GLOBAL FUNCTION TO DELETE TEXT OBJECT IN CASE YOUR CONDITION IS MET 
void removeText(Text*& txt) 
{   
    if (txt->getTitle() == "test" ) { 
        delete txt; // delete the pointer
        txt = 0;//and set it to null
    }
}


int main()
{
    //CREATE A LIST
    std::list<Text*> textList;


    //CREATE TEXT OBJECTS
    Text *t1 = new Text();
    Text *t2 = new Text();
    Text *t3 = new Text();
    Text *t4 = new Text();

    //PUSH BACK THEM INTO LIST
    textList.push_back(t1);
    textList.push_back(t2);
    textList.push_back(t3);


    //CLEAN YOUR LIST FOR THE REQUIRED CONDITION
    for_each(textList.begin(), textList.end(),removeText);

    //USE ERASE - REMOVE IDIOM TO CLEAN YOUR LIST.
    textList.erase(std::remove(textList.begin(), textList.end(),static_cast<Text*>(0)),
               textList.end());

    std::cout<<textList.size()<<std::endl;

}

And the output of above program:

constructor
constructor
constructor
constructor
destructor
destructor
destructor
0

Upvotes: 0

Pavel P
Pavel P

Reputation: 16843

This certainly looks suspicious:

~Text() {delete this;}

Why would you call delete this when this is already being deleted? You store list of pointers to Text. If you create instances of Text yourself, then you need to delete them also (or use some smart pointers to help you do that), or, alternatively you can store list of Text objects:

list<Text> textList;
m_textList.push_back(Text("Some text"));

for (iterator = textList.begin() ; iterator != textList.end() ; ++iterator)
{
    if (iterator->getTitle() == element.toStdString())
    {
        textList.remove(iterator);
        break;
    }
}

Upvotes: 2

sailfish009
sailfish009

Reputation: 2927

use std::unique_ptr, and forget about destruction.

std::vector<std::unique_ptr<Text>> m_text;

std::unique_ptr<Text> text_ptr(new Text());
m_text.push_back(std::move(text_ptr));
//m_text[i]->function();

Upvotes: 1

Related Questions