Do I have to delete this object? (if I don't intend to be ever deleted)

Question

I have programmed a little software and wanted to create a new object on the heap. In the class member function I thus have

void  gShop::CreateCustomer(int type, int number)
    {
    vSlot[number] = new gCustopmer(type);
    vSlot[number]->itsContactToShop=itsShopNumber;
    vSlot[number]->itsNumber=number;
    }

where vSlot is a vector of pointers to customer objects. I have a (here: abbreviated) class gShop, in essence:

class gShop : public gBranch
   {
   public:
       gShop(): vSlot(100){}
      ~gShop(){}   

       std::vector <gCustomer*>   vSlot;
       ...
   }

and in main I call the member function to create new customers..

  vShop[0].CreateCustomer(TYPE_M20, 1);
  vShop[0].CreateCustomer(TYPE_F40, **2**);//EDIT:typo previously here. I intend to create customers by reading a file later on.

  std::cout<< "1" << vShop[0].vSlot[1]->itsTypeString << std::endl;
  std::cout<< "2" << vShop[0].vSlot[2]->itsTypeString << std::endl;

I know that I have created with "new" two objects on the "heap" (if I handle the terminology right - sorry I am quite new to programming without formal education) and I also have two pointers to that objects stored in a vector within the object shop[0].

My question is I heard the saying that for every new there is a delete. Where do I have to delete this object? I am actually not planning on deleting any created shop or customer object in the program.

Secondly, is this code so far okay in terms of not causing memory leaks? I am a bit worried that I created the new object within a member function class, so should I try to implement delete in the destructor to gShop and set the pointer to NULL - in the theoretical case I should ever want to delete shop[0]?

Many thanks.

Answer 1

In the way you have written your code, you should expand you destructor implementation for gShop to iterate over the vector<> vSlot and delete each element. Because you have memory that has to be managed in this way to prevent a memory leak, you are also required to follow the Rule of Three. So, you also need to do something during copy construction (a deep copy), and you have to do something for the assignment operator (cleanup up the vector<> that is about to be copied over, and do a deep copy).

You can avoid these issues, and allow your object to use the default destructor, copy constructor, and assignment operator, by using a smart pointer instead. For example:

    std::vector<std::shared_ptr<gCustomer>> vSlot;

When you create an element in the vSlot, you could use make_shared():

    vSlot[number] = std::make_shared<gCustopmer>(type);

The smart pointer will delete the memory for you when there are no more references to the memory. If you don't have C++.11 available to you, you can use boost::shared_ptr instead.

The smart pointer will make it so that a copy of your gShop will share the same pointers as the original gShop it copied from. The smart pointer makes that situation okay, since there will not be multiple delete calls on the same memory. However, if you need the deep copy semantics, then you would still need to implement your own copy constructor and assignment operator to make the deep copies.

If you want something that will clean up automatically like a smart pointer, but still give you a deep copy with the default copy constructor and default assignment operator, then you can try to use boost::optional.

If you are using g++ version 4.4 or higher, then you should be able to enable C++.11 features with -std=gnu++0x, or -std=c++0x if you don't want GNU extensions. If you have g++ 4.7 or higher, then the options are -std=gnu++11 or -std=c++11.

Answer 2

Yes any memory that is allocated from the heap must be freed! With the "new" you are allocating memory from the heap that is the sizeof gCustomer. A good place to deallocate your memory would be in your destructor: ~gShop() Memory leaks are caused by not deallocating your memory, although once your close your program all the memory is freed automatically.

Answer 3

Every time you create object with new piece of memory is taken from the heap. Only way to communicate with this memory is via pointer you got from new. When you use delete on that pointer memory is freed and may by used for other purposes. So when you lose pointer you create memory leak.
In your code the right way is:
Start with the vector with null pointers. When you create object in exact position check pointer. If it is not null you already have object and have to delete it (or maybe throw error)

void  gShop::CreateCustomer(int type, int number)
{
    if(vSlot[number] != 0) {
         delete vSlot[number];
         vSlot[number] = 0;
    }
    vSlot[number] = new gCustopmer(type);
    vSlot[number]->itsContactToShop=itsShopNumber;
    vSlot[number]->itsNumber=number;
}

When the vector is destroyed you need to free all memory within it. So you destructor will be something like this:

gShop::~gShop() {
    for(int i = 0; i < (int)vSlot.size(); ++i) {
          delete vSlot[i];
    }
}

Answer 4

gShop needs to delete the gCustomer objects that it creates. It can do that in its destructor, eg:

class gShop : public gBranch
{
public:
    ...
    ~gShop()   
    ...
};

gShop::~gShop()
{
    std::vector<gCustomer*>::iterator iter = vSlot.begin();
    std::vector<gCustomer*>::iterator end = vSlot.end();
    while (iter != end)
    {
        delete *iter;
        ++iter
    }
}

Or:

void deleteCustomer(gCustomer *customer)
{
    delete customer;
}

gShop::~gShop()
{
    std::for_each(vSlot.begin(), vSlot.end(), deleteCustomer);
}

However, you would still have a memory leak. You are storing two separate gCustomer objects in the same vSlot[1] slot of vShop[0], so you are losing track of one of your customers. I suspect you meant to do this instead:

vShop[0].CreateCustomer(TYPE_M20, 1);
vShop[0].CreateCustomer(TYPE_F40, 2); // <-- was previously 1

std::cout<< "1" << vShop[0].vSlot[1]->itsTypeString << std::endl;
std::cout<< "2" << vShop[0].vSlot[2]->itsTypeString << std::endl;

That being said, you should re-think your design and let the STL handle all of the memory management for you, eg:

class gShop : public gBranch
{
public:
    std::vector <gCustomer> vSlot;
    ...
};

void gShop::CreateCustomer(int type)
{
    vSlot.push_back(type);
    gCustomer &cust = vSlot.back();
    cust.itsContactToShop = itsShopNumber;
    cust.itsNumber = vSlot.size()-1;
}

vShop[0].CreateCustomer(TYPE_M20);
vShop[0].CreateCustomer(TYPE_F40);


// remember that vectors are 0-indexed
std::cout<< "0" << vShop[0].vSlot[0].itsTypeString << std::endl;
std::cout<< "1" << vShop[0].vSlot[1].itsTypeString << std::endl;

Or:

class gShop : public gBranch
{
public:
    std::vector <std::shared_ptr<gCustomer> > vSlot;
    ...
};

void gShop::CreateCustomer(int type)
{
    std::shared_ptr customer = std::make_shared<gCustomer>(type);
    customer->itsContactToShop = itsShopNumber;
    customer->itsNumber = vSlot.size();
    vSlot.push_back(customer);
}

vShop[0].CreateCustomer(TYPE_M20);
vShop[0].CreateCustomer(TYPE_F40);

std::cout<< "0" << vShop[0].vSlot[0]->itsTypeString << std::endl;
std::cout<< "1" << vShop[0].vSlot[1]->itsTypeString << std::endl;