0

I am trying to improve my 2D array to be created using a class. All methods work fine, it is properly printed out and so on, but when I try to create a destructor I either get an error double free or corruption, I am able to get rid of the error message but then I am not sure whether the memory has been de-allocated properly, because I am using two new keywords but only one delete.

Here is my class declaration:

#pragma once    
class myPlan 
{ 
    int slots; 
    int rows = 3;
  
    int **cell; 
  
public: 
    myPlan(int slots);   // Constructor 
    ~myPlan();
};

and here is definition

#include "myPlan.hpp"

myPlan::myPlan(int slots) 
{ 
    this->slots = slots;
  
    // Create a dynamic array of pointers 
    cell = new int* [rows]; 
  
    // Create a row for every pointer 
    for (int i=0; i<slots; i++) 
    { 
       cell[i] = new int[slots];  
    } 
   for(int i =0; i<3; i++)
   {
    for (int j=0; j<slots; j++) 
    { 
       cell[i][j] = 0;  //setting all cells to zero
    }
   }

} 

//this destructor works
myPlan::~myPlan()
{

   std::cout<<"preparing for destruction"<<std::endl;
        delete[] cell;
   
   std::cout<<"Destroyed class"<<std::endl;
}

//this returns the double free error
/*
myPlan::~myPlan()
{
    for(int i = 0; i < rows; ++i)    
        {
            delete[] cell[i];
        }
        delete[] cell;
}

*/

I know that this is performance-wise slower solution (heap allocation), I tried to use std::vector way.

Thank you very much for any help

Mechatrnk
  • 101
  • 1
  • 13
  • "it did not work" is not a problem description. Why not? What happened? What should happen instead? It looks to me like you just forgot a closing `>`. You should be using `vector` instead of rolling your own dynamic allocation, so just fix your syntax errors. – underscore_d Oct 02 '20 at 08:34
  • Your class has a raw pointer but does not follow the rule of three. https://stackoverflow.com/questions/4172722/what-is-the-rule-of-three It would help to see a [mcve] using your class that causes the issue. – Retired Ninja Oct 02 '20 at 08:35
  • @underscore_d Sorry if I misunderstood you, this is not my problem now, I added the part of std::vector as an attempt to prevent advices for using std::vector I know that it would be better but at the moment I do not know how to do that, I may ask a question how to improve it later but my current problem is about setting a destructor – Mechatrnk Oct 02 '20 at 08:46
  • 1
    your are just one step away from becoming a [three star programmer](https://wiki.c2.com/?ThreeStarProgrammer). Don't use raw owning pointers, use smart pointers and containers, the rule of 0 is your friend – 463035818_is_not_an_ai Oct 02 '20 at 08:48
  • @anastaciu this does not work as well see edited code with second failing destructor – Mechatrnk Oct 02 '20 at 08:50
  • @idclev463035818 as I stated twice, *I know* that I have not used the optimal way of creating 2D array, *I know* that for example std::vector would be better option, but I am new to C++, and I simply *don't know* how to implement 2D std::vector array, even though i tried, but I cannot show you my attempts because I have deleted them and reproducing them would be very very timely for me so *please* either tell me how to fix this destructor or how to completely rewrite my class in order to use stack allocation. This would be more than appreciated. Thank you – Mechatrnk Oct 02 '20 at 08:56
  • I close as typo, @Botje answer is correct. – anastaciu Oct 02 '20 at 08:58
  • 1
    you may not like it but your `myPlan` is not just non-optimal. If you want to juggle with scissors you need to respect the rule of 3/5 as mentioend by Ninja – 463035818_is_not_an_ai Oct 02 '20 at 09:00
  • 1
    btw Its no arrogance when I tell you that this isnt the way. I am not that new to c++ but I don't think I would get a 2d array right by using only raw pointers. Doing the things the hard way has its place in learning, just don't take it too far – 463035818_is_not_an_ai Oct 02 '20 at 09:05
  • @idclev463035818 I did not want to offend you in any way please do not be mad at me, but just when I explicitly state several times "I know that this is not the optimal way" the comment (I paraphrase) "it is not the optimal way" seemed to me very "captain obvious"-like. I am sorry for that, As I said I will try to rewrite it to stack-allocated array, but at the moment it simply was out of my scope. So once again, I apologize if I offended you or something, I am glad that my problem was solved and I can consider this issue closed. – Mechatrnk Oct 02 '20 at 09:11
  • 2
    nevermind, I didn't feel offended. It isnt obvious to anybody who is new to the language that there are so many ways to do it wrong. Beginners often believe that using raw pointers has some value in itself. If it was obvious to you that this isnt the case then nevermind again ;) – 463035818_is_not_an_ai Oct 02 '20 at 09:14

1 Answers1

3

Your constructor allocates a top-level array of size rows (3) but then goes on to fill slots (10) elements of it:

cell = new int* [rows]; 

// Create a row for every pointer 
for (int i=0; i<slots; i++) 
{ 
   cell[i] = new int[slots];  
}

As cell only holds 3 elements, the 7 allocations after firmly corrupt the heap by writing past allocated memory. If you run your code under valgrind or Memory sanitizer, this error should immediately stand out.

Botje
  • 26,269
  • 3
  • 31
  • 41
  • Thank you, finally some constructive help, I need my array to be 3 rows x N columns, but unfortunately I don't know how to fix the destruction. – Mechatrnk Oct 02 '20 at 08:58
  • 1
    If `cell[i]` is allocated using `new int[...]` it should also be freed with `delete[] cell[i]`. – Botje Oct 02 '20 at 09:14