Copy and delete in c++

Question

I have class named "Student" with 3 members.

In addition, I have 2 functions, SetName and Operator=.

class Student
{
private:
    int ID;
    char* name;
    Faculty faculty;

public :
bool SetName(const char* other);

};

typedef enum { ENGINEERING, MEDICINE, HUMANITIES, MANAGEMENT, GENERAL } Faculty;


bool Student::SetName(const char * other)
{

    if (!other)
        return false;
    char* temp;
    temp = new char[strlen(other) + 1];
    if (!temp)
        return false;
    //if (this->name)
        //  delete[]name;

    std::string s = other;
    name = temp;
    memcpy(name,other,strlen(other) + 1);


    return true;
}

Student& Student::operator=(const Student &other)
{

    this->ID = other.ID;
    this->faculty = other.faculty;
    this->SetName(other.name);
    return *this;
};

Of course, I have default and copy destructor. There is a problem with the "SetName" Function, but When I run valgrind, it tells me -

==4661== HEAP SUMMARY:
==4661==     in use at exit: 72,710 bytes in 2 blocks
==4661==   total heap usage: 47 allocs, 45 frees, 74,410 bytes allocated
==4661== 
==4661== 6 bytes in 1 blocks are definitely lost in loss record 1 of 2
==4661==    at 0x4C2E80F: operator new[](unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==4661==    by 0x402798: Student::SetName(char const*) (in /home/noam/Desktop/Avoda3/temp/myStudentSet)
==4661==    by 0x40264E: Student::Student(Student const&) (in /home/noam/Desktop/Avoda3/temp/myStudentSet)
==4661==    by 0x4011D6: StudentSet::Add(Student const&) (in /home/noam/Desktop/Avoda3/temp/myStudentSet)
==4661==    by 0x400F1D: StudentSet::StudentSet(Student const*, int) (in /home/noam/Desktop/Avoda3/temp/myStudentSet)
==4661==    by 0x4020B6: main (in /home/noam/Desktop/Avoda3/temp/myStudentSet)
==4661== 
==4661== LEAK SUMMARY:
==4661==    definitely lost: 6 bytes in 1 blocks
==4661==    indirectly lost: 0 bytes in 0 blocks
==4661==      possibly lost: 0 bytes in 0 blocks
==4661==    still reachable: 72,704 bytes in 1 blocks
==4661==         suppressed: 0 bytes in 0 blocks
==4661== Reachable blocks (those to which a pointer was found) are not shown.
==4661== To see them, rerun with: --leak-check=full --show-leak-kinds=all

When I try to remove the comment and add

 if (this->name)
        delete[]name;

to the SetName function, I have much more problems.

I don't have any idea how to solve this. Can someone help me or give a hint?

Answer 1

For now, i perefer tho use char*

You should prefer to learn how to use your c++ tools. The first suggestion, use std::string would be preferable.

But if you insist on managing memory yourself, you would delete the memory in the class destructor.

~Student( ) { delete[ ] name; }

That way it survives in whatever scope your class object does. But what if you did not call SetName(...)? Then the garbage that name is pointing to is not deletable.

class Student
   :name( nullptr )
{
   ~Student( ) { delete[ ] name; }
   ....

Now if you don't use SetName(...) you won't be trying to delete something unknown.

Answer 2

The problem is that when the created Student object goes out of scope (is destructed), the allocated memory for the name is not deleted. Add a destructor to your class, init the name pointer to NULL in the constructor and test it in the destructor. If not NULL, delete[] it. Good luck, your code might also need some cleaning up but that's another story.

Answer 3

If you want to stick with mix of pre-C+11 C++. You need to make sure that constructors, assignment operators and destructor manage the memory correctly. The following code runs under valgrid without any leaks:

#include <string.h>

typedef enum { ENGINEERING, MEDICINE, HUMANITIES, MANAGEMENT, GENERAL } Faculty;

class Student
{
private:
    int ID;
    char* name;
    Faculty faculty;

public :
    Student() : name(0) {}
    ~Student() {
    if (name)
        delete [] name;
    }
    bool SetName(const char* other);
};

bool Student::SetName(const char * other)
{
    if (!other)
         return false;

    const int len_other_with_null = strlen(other) + 1;
    char *temp = new char[len_other_with_null];

    if (!temp) {
        return false;
    }

    if (this->name)
        delete [] name;

    name = temp;
    memcpy(name, other, len_other_with_null);

    return true;
}


int main(int argc, char ** argv) {
    Student student;
    student.SetName("Hi");

    return 0;
}

Answer 4

The problem with delete [] name you was facing is the direct result of creating a student without a name.

Student student; // student.name is uninitialized. Accessing its value 
                 // would be undefined behaviour.

When you try to use SetName for the first time, you have an uninitialiised value, and deleting it leads to a crash; so you leave out the delete[], and it leads to a memory leak.

Perhaps you expected to student.name to start its life as a null pointer, however this is not how C++ is defined. You must do it yourself.

The idiomatic C++ way is to initialize it in your constructor.

 class Student {
  public:
    Student : name(nullptr) {}
    ...

Now you can uncomment that delete

Let this bug be an important lesson for you. Always write a constructor for your class, and always initialise all data members in it.

Which brings us to the next point. How do you initialize ID and faculty? There are no default "empty" values for them. Even if there were, a student with no ID and no faculty is some kind of ghost we don't want to deal with.

The idiomatic C++ way is to pass these values as parameters to your constructor.

 class Student {
  public:
    Student (int ID_, Faculty faculty_) : 
          name(nullptr), ID(ID_), faculty(faculty_) {}
    ..

But wait, we have created an unnamed student, which may lead to all kinds of problems down the road. Perhaps pass the name to the constructor too?

 class Student {
  public:
    Student (const char* name_, int ID_, Faculty faculty_) :
         name(nullptr), ID(ID_), faculty(faculty_) {
       SetName(name_);
    }
  ...

Now this is a student we can be proud of... except for one thing. We can still pass a null pointer as a name, and the student will be left nameless. In SetName you deal with it by returning false to indicate a failure. Constructors don't return values. What to do?

The idiomatic C++ way is to throw an exception.

 class Student {
  public:
    Student (const char* name_, int ID_, Faculty faculty_) :
         name(nullptr), ID(ID_), faculty(faculty_) {
       if (!name_)
          throw std::domain_error("Null name passed to Student::Student");
       SetName(name_);
    }
  ...

Your class should have a copy constructor according to the rule of three:

 class Student {
  public:
    Student(const Student& other) : 
         Student(other.name, other.ID, other.faculty) {}
    Student (const char* name_, int ID_, Faculty faculty_) :
         name(nullptr), ID(ID_), faculty(faculty_) {
       if (!name_)
          throw std::domain_error("Null name passed to Student::Student");
       SetName(name_);
    }

Of course in real code you want to use std::string rather than manually managing C-style strings.