0

I wrote:

class image {
public:
    linked_list<int, int> UnLabeledList;
    int *SegArray = nullptr;
    int Total_Segments = 0;

    explicit image(int Segments) : SegArray(new int[Segments]) {
        Total_Segments = Segments;
        for (int i = 0; i < Segments; ++i) {
            if (!UnLabeledList.push_back(i, NOT_INIT)) { // allocation failed for one node (Doesn't throw any exception)
                ~UnLabeledList();
                delete[] SegArray;
                throw;
            }
            SegArray[i] = NOT_INIT;
        }
    };
};

In case one allocation failed I want to destroy the object (since it has previously allocated nodes) for example when allocation fails at i==5. How can I call the d'tor of UnLabeledListto prevent memory leak?

Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
Sholy
  • 43
  • 4
  • What does `~UnLabeledList();` do? – Scott Hunter Sep 04 '21 at 08:06
  • 1
    You don't have to, it is guaranteed to be destroyed automatically. Also, if you use a `std::vector` for `SegArray`, then you don't have to call `delete` on it either. – G. Sliepen Sep 04 '21 at 08:06
  • It will get destroyed when image is destroyed – cup Sep 04 '21 at 08:07
  • Keep reading your C++ tutorial, it should explain these concepts. If you don't have a good learning source, check out https://stackoverflow.com/questions/388242/the-definitive-c-book-guide-and-list. Also, just a hint: I don't remember ever using `new[]`. All cases where you're tempted to do so, you should probably use a `std::vector<>` instead. – Ulrich Eckhardt Sep 04 '21 at 08:09
  • `UnLabeledList` will get destroyed when the `image` is destroyed. If you actually have memory leaks, they are not caused by lack of destruction. – molbdnilo Sep 04 '21 at 08:15
  • @ScottHunter my try :) – Sholy Sep 04 '21 at 08:15
  • You really should read about what the stack and what the heap is. Stack variables are automatically destroyed. Yours is a stack variable. Other than that, for a heap one: `MyClass* object = new MyClass; delete object;`. The `delete` here is what calls the dtor. Note that if you write code in which you need to use `delete`, you set yourself up to errors since that is easily forgotten. Better is to use smart pointers (or usual stack variables, if you are able to). – Aziuth Sep 04 '21 at 10:50
  • @Aziuth "*You really should read about what the stack and what the heap is*" - more accurately, you should read about what object lifetime is, particularly in the cases of automatic lifetime and dynamic lifetime. For instance, `UnLabeledList` has automatic lifetime regardless of whether `image` is constructed on the stack or the heap. Besides, Stacks and Heaps are merely *implementation details* chosen by the compiler on specific platforms. Not all platforms have stacks and heaps. So, learn how the C++ language defines lifetime, not how the underlying platform implements lifetime. – Remy Lebeau Sep 04 '21 at 18:13

2 Answers2

1

You can't directly invoke the destructor of the UnLabeledList member like that.

If the image constructor throws an uncaught exception, the UnLabeledList member will be destructed automatically, if it was successfully constructed before the exception was thrown.

The correct solution is to implement a clear() method in the linked_list class, which you can call whenever needed, including in the copy constructor and destructor. For example:

template<...>
class linked_list {
    // ...
public:
    linked_list() {
        // initialize the list as needed...
    }

    linked_list(const linked_list &src) : linked_list() {
        for (each node in src) {
            if (!push_back(...)) {
                clear();
                throw ...;
           }
        }
    }

    ~linked_list() {
        clear();
    }

    linked_list& operator=(const linked_list &rhs) {
        // clear and copy the list as needed...
    }

    void clear() {
        // free nodes as needed...
    }

    node* push_back(...) {
        // add new node as needed...
        return ...; // nullptr on error
    }

    //...
};

class image {
public:
    linked_list<int, int> UnLabeledList;
    int *SegArray = nullptr;
    int Total_Segments = 0;

    explicit image(int Segments) : SegArray(new int[Segments]) {
        Total_Segments = Segments;
        for (int i = 0; i < Segments; ++i) {
            if (!UnLabeledList.push_back(i, NOT_INIT)) {
                delete[] SegArray;
                throw ...; // <-- UnLabeledList::~linked_list() called automatically!
            }
            SegArray[i] = NOT_INIT;
        }

    ~image() {
        delete[] SegArray;
    } // <-- UnLabeledList::~linked_list() called automatically!
};

You also can't call a parameter-less throw; outside of an active catch handler. Outside of a catch, you must throw an actual value/object explicitly.

if (!UnLabeledList.push_back(i, NOT_INIT)) {
    delete[] SegArray;
    throw std::runtime_error("can't push a new node");
}
Remy Lebeau
  • 555,201
  • 31
  • 458
  • 770
  • "The correct solution is to implement a clear() method in the linked_list class" but that doesn't solve the problem, can you show example? deleting each single node does not delete the whole object... – Sholy Sep 04 '21 at 08:19
  • @Sholy "*but that doesn't solve the problem*" - yes, it does, if the linked_list class is implemented properly so it is always in a stable and valid state. I have added an example. – Remy Lebeau Sep 04 '21 at 08:57
1

You should never call a destructor yourself to clean up things. The language rules take care of it.

It is recommended to use standard classes like std::vector and std::list rather than raw arrays/pointers and homebrew lists. The standard classes already take care of exceptions and cleanup, so your class should look something like

class image {
public:
    std::list<std::pair<int, int>> UnLabeledList;
    std::vector<int> SegArray;
    int Total_Segments = 0;

    explicit image(int Segments) : SegArray(Segments, NOT_INIT), TotalSegments(Segments) {
        for (int i = 0; i < Segments; ++i) {
            UnLabeledList.push_back({i, NOT_INIT});
        }
    };
};
n. m. could be an AI
  • 112,515
  • 14
  • 128
  • 243
  • sorry that's the point of my whole question to use my own implementation, I would be glad if someone can show how can I edit my current code not totally replace it – Sholy Sep 04 '21 at 08:47
  • @Sholy If you want to learn how to roll your own implementation, *implement your own linked list correctly such that it is as safe to use as std::list* and *implement your own vector correctly such that it is as safe to use as std::vector*. Do not put exception handling and clean up into higher level class like `image`. A class should do one thing. Taking care of the image functionality is enough. There is no need to mix in exception handling and clean up. It is needlessly hard, it is error prone, and it is not done in real world programming. – n. m. could be an AI Sep 04 '21 at 08:57