How to define the destructor to safely free a dynamic array-CodePudding

So, for instance, I have the following code which I want a object's pointer member to point to a memory which was pointed by another temporary object's member.

struct A {
    int * vals;
    A(): vals(new int[10]) { }
    ~A(){ delete[] vals; }
};

int main() {
    A a;
    {
        A temp;
        for (int i = 0; i < 10;   i) {
          temp.vals[i] = 100;
        }
        a.vals = temp.vals;
        temp.vals = nullptr;  // avoid double free
    }

I set temp.vals to nullptr in case the destructor of temp will free that memory. So far so good, I guess. However, if I change the vals to a dynamic array, i.e. a pointer to pointers:

struct A {
    int ** vals;
    
    A(): vals(new int*[10]) {
        for (int i = 0; i < 10;   i) {
            vals[i] = new int;
        }
    }
  
    ~A(){
      for (int i = 0; i < 10;   i) {
          delete vals[i]; // illegal to dereference nullptr
      }
      delete [] vals;
    }
};

int main() {
    A a;
    {
        A temp;
        for (int i = 0; i < 10;   i) {
            temp.vals[i] = new int(1);
        }
        a.vals = temp.vals;
        temp.vals = nullptr; // avoid double free
    }
}

I have add a for loop in destructor to handle the nested allocated memory, and to avoid the memory be freed by the destructor of temp, I set temp.vals to nullptr, which, however will cause a segmentation fault since when destructor of temp is called, it is illegal to dereference a nullptr.

So my question is, how to correct set the destructor to handle the dynamic array.

I'm not a native speaker, so please forgive my grammar mistakes.

CodePudding user response：

The typical C solution looks a bit different:

class A {
  private:
     int* vals;
  public:
    A(): vals(new int[10]) { }

    ~A(){ delete[] vals; }
    A (A const& src); // Copy constructor
    A (A&& src) : vals (src.vals) { src.vals = nullptr; }
    A& operator=(A const&); // Assignment
    A& operator=(A &&);
};

You can now write a = std::move(temp). Outside the class, you don't need to know how the inside works.

For your 2D array, just define the same special member functions. This is usually called the "Rule of Five". If you need a destructor, you probably need the other 4 functions as well. The alternative is the "Rule of Zero". Use std::vector or another class that manages memory for you.

CodePudding user response：

However, if I change the vals to a dynamic array, i.e. a pointer to pointers

In the first program, vals is a pointer. It points to a dynamic array of integers. In the second program, vals is also a pointer. It points to a dynamic array of pointers.

how to correct set the destructor to handle the dynamic array.

You set vals to null. If null is a valid state for vals, then it isn't correct to unconditionally indirect through it in the destructor. You can use a conditional statement to do so only when vals isn't null.

However, the program is hardly safe because vals is public, and thus it is easy to mistakenly write a program where it is assigned to point to memory that isn't owned by the object. In cases where destructor cleans up an owned resource, it is important to encapsulate the resource using private access to prevent accidental violation of class invariants that are necessary to correctly handle the resource.

Now that vals is no longer outside of member functions, you cannot transfer the ownership like you did in your example. The correct way to transfer the ownership is to use move assignment (or constructor). The implicitly generated move constructor cannot handle an owning bare pointer correctly. You must implement them, as well as the copy assignment and constructor.

Furthermore, you should use an owning bare pointer in the first place. You should use a smart pointer instead. If you simply replaced the bare pointers with unique pointer, then the implicitly generated move assignment and constructor would handle the resource correctly, and the copy assignment and constructor would be implicitly deleted.

Lastly, the standard library has a container for dynamic arrays. Its called std::vector. There's typically no need to attempt to re-implement it. So in conclusion, I recommend following:

std::vector<int> a;
{
    std::vector<int> temp;
    for (int i = 0; i < 10;   i) {
        temp.vals[i] = 1;
    }
    a = std::move(temp);
}

There are still issues such as the temporary variable being entirely unnecessary, and the loop could be replaced with a standard algorithm, but I tried to keep it close to the original for the sake of comparison.

P.S. It's pretty much never useful to dynamically allocate individual integers.