C++ class destructor not working properly

Question

#include<iostream>
#include<cstring>
using namespace std;
class Animal
{
protected:
int age;
char* name;
public:
Animal()
{
    name=new char[1];
    age = 0;
    strcpy(name," ");
}
Animal(int _age, char* _name)
{
    age=_age;
    name = new char[strlen(_name)+1];
    strcpy(name, _name);
}

~Animal()
{
    delete[] name;
}

friend istream& operator >>(istream& in, Animal& a);
friend ostream& operator <<(ostream& out,  const Animal& a);

};
 istream& operator >>(istream& in, Animal& a)
{
    in>>a.name>>a.age;
    return in;
}
 ostream& operator <<(ostream& out, const Animal& a)
{
    out<<a.name<<a.age;
    return out;
}


int main()
{
Animal a;
cin>>a;
cout<<a;
return 0;
}

This piece of code gives me the opportunity to enter a, then prints it and then the screen freezes and stops working. If I delete the destructor, it works properly. Why is this happening? And is it because of the destructor really?

Answer 1

If you really don't want to use std::string, your best bet is something in the line of (live at coliru):

#include<iostream>
#include<cstring>

using namespace std;

class Animal {
private:
  // copy a string
  inline static char* dstr(const char* string) {
    if( !string ) return NULL;
    size_t l = strlen(string);
    if( !l ) return NULL;
    return strcpy(new char[++l], string);
  }
protected:
  int age;
  char* name;
public:
  // initialize an "empty" Animal
  Animal() : age(0), name(NULL) {}

  // initialize an animal by age and name
  Animal(int _age, const char* _name): age(_age), name(dstr(_name)) {}

  // initialize an animal from another animal:
  // copy the name string
  Animal(const Animal& _a): age(_a.age), name(dstr(_a.name)) {}

  // assign an animal from another animal:
  // first delete the string you have, then copy the string
  Animal& operator=(const Animal& _a) {
    // for exception-safety, save the old "name" pointer,
    // then try to allocate a new one; if it throws, nothing happens
    // to *this...
    char* oldname = name;
    name = dstr(_a.name);
    age = _a.age;
    delete[] oldname;
    return *this;
  }

  // if C++11
  // we have something called "move" constructor and assignment
  // these are used, for instance, in "operator>>" below
  // and they assume that _a will soon be deleted
  Animal(Animal&& _a): Animal() {
    swap(age, _a.age);
    swap(name, _a.name);
  }
  Animal& operator=(Animal&& _a) {
    swap(age, _a.age);
    swap(name, _a.name);
    return *this;
  }
  
  ~Animal() { delete[] name; }

  friend ostream& operator <<(ostream& out,  const Animal& a);

};

istream& operator >>(istream& in, Animal& a) {
  const size_t MAX_ANIMAL_NAME = 2048;
  int age;
  char n[MAX_ANIMAL_NAME+1];
  if( in.getline(n, MAX_ANIMAL_NAME) >> age )
    a = Animal(age, n);
  return in;
}

ostream& operator <<(ostream& out, const Animal& a) {
  return out<<a.name<<endl<<a.age<<endl;
}


int main() {
  Animal a { 23, "bobo" };
  cout<<a;
  cin>>a;
  cout<<a;
}

This does not leak memory, does not have undefined behaviours, and does not have buffer overruns.

You can also segregate the "need to manage memory" to a separate class:

#include<iostream>
#include<cstring>

using namespace std;

class AnimalName {
  private:
    char *n;
    inline static char* dstr(const char* string) {
      if( !string ) return NULL;
      size_t l = strlen(string);
      if( !l ) return NULL;
      return strcpy(new char[++l], string);
    }
  public:
    AnimalName() : AnimalName(NULL) {}
    AnimalName(const char *_n) : n(dstr(_n)) {}
    AnimalName(const AnimalName& _n) : n(dstr(_n.n)) {}
    // see exception-safety issue above
    AnimalName& operator=(const AnimalName& _n) { char *on = n; n = dstr(_n.n); delete[] on; return *this; }
    AnimalName(AnimalName&& _n) : AnimalName() { swap(n, _n.n); }
    AnimalName& operator=(AnimalName&& _n) { swap(n, _n.n); return *this; }
    ~AnimalName() { delete[] n; }

    operator const char*() const { return n; }
    friend istream& operator>>(istream& i, AnimalName& n) {
      const size_t MAX_ANIMAL_NAME = 2048;
      char name[MAX_ANIMAL_NAME+1];
      if( i.getline(name, MAX_ANIMAL_NAME) )
        n = name;
      return i;
    }
};

class Animal {
protected:
  int age;
  AnimalName name;
public:
  // initialize an "empty" Animal
  Animal() : age(0) {}

  // initialize an animal by age and name
  Animal(int _age, const char* _name): age(_age), name(_name) {}

  friend ostream& operator <<(ostream& out,  const Animal& a) {
    return out<<a.name<<endl<<a.age<<endl;
  }
};

istream& operator >>(istream& in, Animal& a) {
  AnimalName n;
  int age;
  if( in >> n >> age )
    a = Animal(age, n);
  return in;
}


int main() {
  Animal a { 23, "bobo" };
  cout<<a;
  cin>>a;
  cout<<a;
  return 0;
}

This way you get to follow the "rule of zero" (basically, classes that do not have the sole responsibility of managing memory/resources should not manage memory and therefore should not implement copy/move-constructors, assignments, or destructors.)

And that takes us to the real reason why you should use std::string: it not only does the memory management for you, but it also takes good care of your IO needs, eliminating the need for a "maximum animal name" in your example:

#include<iostream>
#include<string>

using namespace std;

class Animal {
protected:
  string name; // name first, for exception-safety on auto-gen assignment?
  int age;
public:
  // initialize an "empty" Animal
  Animal() : age(0) {}

  // initialize an animal by age and name
  Animal(int _age, const string& _name): age(_age), name(_name) {}

  friend ostream& operator <<(ostream& out,  const Animal& a) {
    return out<<a.name<<endl<<a.age<<endl;
  }
};

istream& operator >>(istream& in, Animal& a) {
  string n;
  int age;
  if( getline(in, n) >> age )
    a = Animal(age, n);
  return in;
}


int main() {
  Animal a { 23, "bobo" };
  cout<<a;
  cin>>a;
  cout<<a;
}

Answer 2

There are several bugs in your code. The first one is in the constructor

Animal()
{
    name=new char[1];
    age = 0;
    strcpy(name," ");
}

String literal " " consists from two characters: the space character and the terminating zero '\0;. So you need to allocate dynamically 2 bytes

    name=new char[2];

that to use after that function strcpy.

Or instead of string literal " " you should use "an empty" string literal "" that contains only the terminating zero '\0'.

The other bug in function

 istream& operator >>(istream& in, Animal& a)
{
    in>>a.name>>a.age;
    return in;
}

As you initially allocated only 1 byte pointed to by name then you may not use operator

    in>>a.name;

because you will overwrite memory that does not belong to the allocated extent.

For example you could define the operator the following way

std::istream& operator >>( std::istream& in, Animal &a )
{
    char itsName[25]; 

    in >> itsName >> a.age;

    char *tmp = new char[std::strlen( itsName ) + 1];

    std::strcpy( tmp, itsName );

    delete [] name;

    name = tmp;

    return in;
}

In this case you could enter a name that does not exceed 24 characters.

Take into account that you need also to define a copy constructor and the copy assignment operator if you are going to assign one object to another.

Answer 3

You allocate a C-string having the size 1 and copy the C-string " " having the size 2 to it. Also you read an unknown amount of characters to the name in 'istream& operator >>(istream& in, Animal& a)`. Both will corrupt the memory the name is pointing to and both can be easily fix by using std::string:

class Animal
{
    protected:
    int age;
    std::string name;

    public:
    Animal()
    : age(0)
    {}

    Animal(int age_, std::string name_)
    : age(age_), name(name_)
    {}
};

This avoids writing a destructor and copy-constructor and assignment operator, which are missing in your code (See: Rule of three).

Answer 4

A simple fix is to use std::string for your strings.

It almost doesn't matter what the specific errors you get are. But just to cover that, already in the constructor of Animal,

Animal()
{
    name=new char[1];
    age = 0;
    strcpy(name," ");
}

you have Undefined Behavior by allocating just a single element array and then using strcpy top copy two char values there. Overwriting some memory after the array.

Then in operator>> the UB trend continues.

And so forth.

Use std::string.

Answer 5

Your memory management is wrong, which is corrupting the memory. You are allocating space for one character for name. But

strcpy(name," ");

will pass beyond the memory you allocated, since cstring is null terminated, it will put actually two character, effectively corrupting your memory ( you are accessing memory that is not allocated by your program). It itself has undefined behavior.

Further you are deleting an apparently unknown amount of memory in the destructor, which has also undefined behavior.