1

I have tried to read lots of solutions to memory leaks occur in copy constructor, but i still didnt understans how to solve it.

For example i have a "Person" class that has those mambers and functions (header files):

#include "Object.h"
#include <string.h>
#include <iostream>
using namespace std;
class Person: public Object
{

private:
    char * p_name;
    int  length;
public:
    virtual Object * copy() const;
    virtual void print(ostream & os) const;
    Person(char * name);
    Person(const Person & per);
    ~Person();
};

In this program Im trying to enter "Objects" to a Vector while Person and Vector inherit from Object. In both of the copy const i have memory leak problems (the program is working excellent).

For example in this code im getting a memory leaks of all of those 5 char arrays. I have more problems also in Vector memory leaks, but lets start in this simple code in the main (that occurs 5 memory leaks of the char array):

int main ()
{
    const int SIZE = 5;
    Person* persons[SIZE];
    int i;

    // preparation of name array 
    for (i = 0; i<SIZE; i++) {
        char* tmp = new char[10];
        sprintf(tmp, "P-%d", i);
        persons[i] = new Person(tmp);
    }

    for (i = 0; i < SIZE; i++)
        delete persons[i];

    return 0;
}

Person class is:

#include "Person.h"
using namespace std;



Object * Person::copy() const
{
    Person * p = new Person(*this);
    return p;
}

void Person::print(ostream & os) const
{
    for (int i = 0; i < this->length-1; i++)
    {
        os << this->p_name[i];
    }
}


Person::Person(char * name)
{
    delete this->p_name;
    this->length = strlen(name)+1;
    p_name = new char[length];
    strncpy(p_name, name, length);
}

Person::Person(const Person & per)
{
    delete[] this->p_name;
    this->length = strlen(per.p_name) + 1;
    this->p_name = new char[this->length];
    strncpy(this->p_name, per.p_name, this->length);
}

Person::~Person()
{
    delete[] this->p_name;
}

I would be thankful for your help!!

Ben
  • 201
  • 1
  • 4
  • 13
  • 6
    This would be far easier if you switched to `std::string`. – Cory Kramer Jan 13 '16 at 12:40
  • 1
    first of all you are not deleting the tmp arrays you allocated in main so that is probably one of the memory leaks you have. – Jonathan Jan 13 '16 at 12:43
  • This link may help you to understand how efficiently you can write copy constructor http://stackoverflow.com/questions/3279543/what-is-the-copy-and-swap-idiom – CreativeMind Jan 13 '16 at 12:46
  • 1
    You need to make your destructor **virtual** (In your `Object` class as well as in your `Person` class). – Naios Jan 13 '16 at 12:48
  • 1
    @Rabbid76 Doesn't help. It's still undefined behavior because `p_name` is uninitialized. – Emil Laine Jan 13 '16 at 12:48
  • 1
    If managing memory manually is _this_ hard, you should just stick to `std::string` and `std::make_unique` etc. and never use `new` again. – Emil Laine Jan 13 '16 at 13:01
  • @DenisBlank Thx! i didnt know it. Thx for all of you or your attention. – Ben Jan 13 '16 at 15:28

4 Answers4

3

You don't have just a memory leak. You have a full-fledged, memory-corrupting, undefined behavior:

Person::Person(char * name)
{
    delete this->p_name;

This is your constructor. The p_name class member does not appear to be initialized in any way. And the first thing you do, is you try to delete it.

So, whatever random value p_name will contain, as a result of the most recent set of cosmic rays striking the RAM capacitors that hold its initial value, the first thing the constructor does is try to free the pointed-to memory.

Before you can worry about any alleged leaks from your copy constructor, you need to fix a bunch of problems, like this, elsewhere.

Sam Varshavchik
  • 114,536
  • 5
  • 94
  • 148
3

In main(), the tmp char array is not deleted, and that's the first memory leakage I'm seeing.

In the Person(char * name) constructor you call a delete on

Person::Person(char * name)
{
     delete this->p_name;

the p_name is not allocated, so the behavior is undefined. And p_name is an array, so delete[] should be used.

if you use the std::string class, you can at least avoid the confusion between delete and delete[]

Luca Pizzamiglio
  • 313
  • 1
  • 7
  • Thanks, so there is a way to delete it through the program or i should delete it just from main? – Ben Jan 13 '16 at 14:07
  • Every `new` has to have a corresponding `delete`. If you call the `new` at the beginning of the loop, you should call the `delete` at the end of the loop. Note: in this case, a `delete[]` – Luca Pizzamiglio Jan 13 '16 at 14:08
  • Yeah right. this was a tester sent by my teacher, so i was kind of blind and didn't see it. Thx for your help!! i really appriciate it!! – Ben Jan 13 '16 at 15:24
1

in main(), where you prepare your c you allocate 5 char buffers, that're never freed

also (not speaking of std::string which would help you with your string stuff) , get rid of the unneeded "this->"s:

Person::Person(const Person & per)  
{
    delete[] this->p_name;
    this->length = strlen(per.p_name) + 1;
    this->p_name = new char[this->length];
    strncpy(this->p_name, per.p_name, this->length);
}

could look like this:

Person::Person(const Person & per)
{
    delete[] p_name;
    length = strlen(per.p_name) + 1;
    p_name = new char[length];
    strncpy(p_name, per.p_name, length);
}
Tommylee2k
  • 2,683
  • 1
  • 9
  • 22
-1

There are some problems with delete.

In your main, in the first while you have to delete temp; in the second well just don't use it, use delete [] persons.

In your copy constructor don't use delete this->p_name, that's not correct.

At first you have to set the pointer to null so p_name=NULL, and then use setters and getters, same for your value constructor.

And in your destructor, test if p_name exists before deleting it.

CreativeMind
  • 897
  • 6
  • 19
Remi Hirtz
  • 134
  • 3
  • 16