Fixing memory leak

[qwerty123456]

I'm having trouble fixing a memory leak in my code. Based on using valgrind, the memory leak is occurring on line 6 in the Student constructor, but I'm not sure how to delete the new array I made on the heap because I tried to delete in the destructor, but it isn't working. Any help is appreciated.

  #include "student.h"
  #include <string>
  #include <cstring>
  #include <sstream>
  Student::Student(const char * const name, int perm) {
        this->name = new char[strlen(name) +1];
        strcpy(this->name, name);
        this->perm = perm;
  }

  int Student::getPerm() const {
        return perm;
  }

  const char * const Student::getName() const {
        return name;
  }

  void Student::setPerm(const int permNumber) {
        this->perm = permNumber;
  }

  void Student::setName(const char * const name) {
        this->name = new char[strlen(name)+1];
        strcpy(this->name, name);
  }


  Student::Student(const Student &orig) {
        this->setName(orig.getName());
        this->setPerm(orig.getPerm());
  }

  Student::~Student() {
        delete [] name;

[keskiverto]

What happens on line 24?


That is not all. You should read: http://en.cppreference.com/w/cpp/language/rule_of_three



PS. There is a difference between initialization and the change of existing objects.

Student::Student(const char * const name, int perm)
 : name( new char[strlen(name) +1] ),
   perm( perm )
{
  strcpy( this->name, name );
}
// vs
Student::Student(const char * const name, int perm)
{
  this->name = new char[strlen(name) +1];
  strcpy(this->name, name);
  this->perm = perm;
}

Prefer the member initialization list syntax

[qwerty123456]

Oh sorry I forgot to include the rest of the code which has the overloaded assignment operator

Student & Student::operator=(const Student &right) {
  // The next two lines are standard, and you should keep them.
  // They avoid problems with self-assignment where you might free up
  // memory before you copy from it.  (e.g. x = x)

        if (&right == this)
                return (*this);

  // TODO... Here is where there is code missing that you need to
  // fill in...
        this->setName(right.getName());
        this->setPerm(right.getPerm());



  // KEEP THE CODE BELOW THIS LINE
  // Overloaded = should end with this line, despite what the textbook says.
        return (*this);

}

std::string Student::toString() const {
        std::ostringstream oss;

        oss << "["
            << this->getName() << ","
            << this->getPerm() << "]";
        return oss.str();

}

On line 24, it takes in a name and sets the Student's private variable of "name" to be what is taken in.

[keskiverto]

Lets do:

int main() {
  Student snafu( "John", 42 );
  snafu.setName( "Dolly");
}

What does happen there? Calls to constructor, setName, and destructor.

What of interest does happen in them?

snafu.name = new char[strlen("John") +1]; // line 6
snafu.name = new char[strlen("Dolly") +1]; // line 24
delete [] snafu.name;

The constructor allocates a block of memory. Lets call it 'foo'.
The setName allocates a block of memory. Lets call it 'bar'.
The destructor deallocates a block. Which block? The bar.

What did happen to the 'foo'?

[qwerty123456]

So the new char[] in the constructor wouldn't be affected by the destructor. If I were to change the constructor to be in the member initialization syntax, would this change anything?

[keskiverto]

No, that won't, but it is still recommended to do, because in some cases you absolutely have to use member initialization list. Why not make it a habit from day one?

A real issue is here:

void Student::setName(const char * const name)
{
  // this->name points to some memory block 'foo'
  this->name = new char[strlen(name)+1];
  // nothing points to memory block 'foo' any more
  strcpy( this->name, name );
}

The previously allocated block "foo" leaks every time you call setName().

You should deallocate in setName().


However, what if strlen(name) <= strlen(this->name)?
Do you still have to deallocate&allocate?

[qwerty123456]

How would I deallocate in setName()?

void Student::setName(const char * const name)
{
  
  this->name = new char[strlen(name)+1];
  delete [] name;
  strcpy( this->name, name );
}

This is resulting in a seg fault when I try to test the code.

[keskiverto]

void Student::setName(const char * const name)
{
  // this->name points to some memory block 'foo'
  this->name = new char[strlen(name)+1];
  // this->name points to different memory block 'bar'
  // the 'foo' has already leaked

  delete [] name; // attempts to deallocate argument the 'name', not the this->name
  // if you try 'delete [] this->name' here, you deallocate the 'bar'

  strcpy( this->name, name );
}

Think also the order of operations.

[qwerty123456]

Thank you for all the help. I was able to fix the memory leaks.