-2

I am practicing some inheritance and virtual destructors and i keep getting problem with exception being thrown after running my code, where child class destructor works properly but superclass destructor just wont work properly.

I suppose that i have a few misconceptions about destructors since every time i saw virtual destructors, command 'delete' was used outside of destructor actually, but, if i do that, then, what is the point of creating destructor?

#include <iostream>


template <class T>
class List
{
protected:
    T *data;



public:
    List()
    {

        data = new T[5];
        std::cout << "List constructor!";
    }


    virtual void putIn()
    {
        for (int i = 0; i < 4; i++)
            std::cin >> data[i];
    }
    virtual void printOut()
    {
        for (int i = 0; i < 5; i++)
            std::cout << data[i] << std::endl;
    }
    virtual ~List()
    {
        delete[]data;
        std::cout << "List destructor!";
    }


};


template <class T>
class League : public List<T>
{
public:
    League()
    {

        data = new T[5];
        std::cout << "League constructor!";
    }


    virtual void putIn()
    {
        std::cout << "Enter your values:\n";
        for (int i = 0; i < 5; i++)
            std::cin >> data[i];
    }
    virtual void printOut()
    {
        std::cout << "Your values are:\n";
        for (int i = 0; i < 5; i++)
            std::cout << data[i] << std::endl;
    }


   ~League()
    {
        delete[]data;
        std::cout << "League destructor!";
    }

};



int main()
{
    List<char> *p;
    League<char> leag;

    p = &leag; 

    p ->putIn();


    getchar();
    getchar();




}

Everything works just fine, but when program finishes, it says that exception is thrown it points to the base class destructor. Any help appreciated!

cdummie
  • 163
  • 1
  • 6
  • You're violating [The Rule of Three](https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three), but that's not actually the source of the bug. – Mooing Duck Feb 02 '19 at 18:59
  • 2
    Make `data` private - protected data memebers are always a problem, as you have found. –  Feb 02 '19 at 19:00
  • @MooingDuck So, problem occurs because i don't have a copy assignment operator overloaded? – cdummie Feb 02 '19 at 19:02
  • @NeilButterworth why would protected data members cause any problems? What do you mean by that? – cdummie Feb 02 '19 at 19:03
  • as I understand you try to delete `data` twice. @cdummie private members would avoid a derived class to try and access or delete them (as in your case). – Philipp Feb 02 '19 at 19:03
  • 3
    If you had made `data` private, you would not be able to delete it twice. –  Feb 02 '19 at 19:04
  • @Philipp well, it looks like i do, but in reality i just wanted to create destructors that actually do something, what is the point of empty destructor? (I am still new to inheritance stuff) – cdummie Feb 02 '19 at 19:05
  • 2
    If you have a `delete` in the destructor, then you must ensure that you do not have a default copy constructor and/or a default assignment operator, as this can copy the pointer and you will have double frees. And you should only `delete` pointers you own. But modern c++ code you should not use `delete` and `new` anyway. – t.niese Feb 02 '19 at 19:05
  • @cdummie: There is no point to an empty destructor, unless it's a base class, in which case you should probably make the destructor `virtual`, even if it's empty. – Mooing Duck Feb 02 '19 at 19:09
  • Do not create a destructor if it's going to be empty. If a class does not have members that should be cleaned up then there is no need for a destructor. `List` class needs it because it has `data`, but `League` does not have such members, so there is no no need for `~League()`. – Dialecticus Feb 02 '19 at 19:10
  • 1
    As for the deletes in the destructors, the key notion here is **ownership** -- who is the owner of `data`, and, hence, who is responsible for managing its lifetime? Since it's a member of `List`, the obvious owner is `List`. The destructor for `List` should delete it, and derived classes should not. They can look at it and what it points at, but should not try to manage its lifetime. – Pete Becker Feb 02 '19 at 19:10

3 Answers3

2

The summary of the crash is: When a derived class is destroyed, it's destructor is called, and then it's based class destructor is called. The derived destructor is calling delete[]data; and then the base class is calling delete[]data; a second time, which is undefined behavior. Luckily, in this case, it resulted in a crash, so you knew to look for it.

The idea is that the base class owns the data pointer, and so the base class should be the one to delete it. The derived class doesn't own the pointer, and so should not be deleting it.

Due to this misunderstanding, Dialectius also observed a memory leak. When the League is constructed, the List constructor is called first, which allocates memory, and then the League constructor is called, which allocates different memory, leaking the first bit of memory. Again, the derived class doesn't own the pointer and should probably not be touching it. The derived class should ask the base class to modify the pointer.

It's also worth mentioning that your classes violate The Rule of Three and so you should add copy constructors and copy assignment operators to keep the class safe.

Mooing Duck
  • 64,318
  • 19
  • 100
  • 158
  • but then, what is the point of derived-class destructor if it is not going to delete it? I could then create only regular base class destructor the usual way, right? – cdummie Feb 02 '19 at 19:15
  • @cdummie: Correct: Since your derived class owns no resources, there's no reason for it to make have a destructor. Since your base class is designed to be inherited from, its' best to mark it's destructor as `virtual`, even if it's empty. – Mooing Duck Feb 02 '19 at 19:16
  • 1
    @cdummie See [the Rule of Zero](https://en.cppreference.com/w/cpp/language/rule_of_three) for a more general case of "If you don't have to do anything, do nothing." – user4581301 Feb 02 '19 at 19:20
1

The problem is that data get deleted twice, first in ~League() and then in ~List(). Remove delete from ~League().

Then there is a problem of memory leak. League is not responsible for List::data but it initializes it. This causes a memory leak, because previous value will be lost and will not be deleted, ever. So don't set a new value in League, and use some other way to tell List to change the value, and then only List is responsible for its member, and would have to delete[] the old one before setting a new one.

Dialecticus
  • 16,400
  • 7
  • 43
  • 103
  • Why do i need child class destructor then? – cdummie Feb 02 '19 at 19:15
  • In this example you don't need it. Remove it. Do you want to make an example where you do need a destructor? – Dialecticus Feb 02 '19 at 19:20
  • Actually, i tried, and i thought that this is one of those, however, i was wrong, and i can't think of any examples where destructor in child class is needed. – cdummie Feb 02 '19 at 21:32
  • 1
    Suggesting examples is not in the scope of Stack Overflow site. `new` and `delete` should be already very rarely used in modern C++ (there are `std::shared_ptr` and `std::unique_ptr` that take care of allocation and deallocation for you). A league is not a list anyway. It may contain a list of some sort (so a member of the `League` class would be some `List`), but it is not a list on its own. But, to give an example anyway, there could be players and referees in this league, all of them employees. All employees have a salary, but only players can have a club affiliation. – Dialecticus Feb 03 '19 at 00:47
  • I'll keep that in mind and i will try to improve this code based on your comment. Thanks for your effort, i appreciate it! – cdummie Feb 03 '19 at 11:30
1

Why do I need child destructor then ?

Like Dialecticus said you don’t need it in this situation. Suppose that you want to keep backup of your array then you’ll have another dynamic templated field in child class like T *childData. Now it’s good idea to use child class’ destructor.

patoglu
  • 401
  • 4
  • 16