0

Hello I'm new to c++ and trying to grasp the memory management in it with the free() and delete. I have this add_flat function which works fine until I try to free the memory. I created an FlatList object and added the flats .Without the delete statement it works fine but after I put it the Head just returns some garbage value. Should I define a destructor ? I'm very new in c++ so any help would be appreciated.

void FlatList::add_flat(int index,int initial_bandwith,int flat_id) {

    Flat* new_flat = new Flat() ;

    new_flat->id = flat_id ;
    new_flat->initial_bandwidth = initial_bandwith ;
    new_flat->is_empty = false ;

    Flat* current = Head ;

    if (index == 0 ) {

        new_flat->next_Flat = Head ;
        Head->prev_Flat = new_flat ;
        Head = new_flat ;

    }

    else {

        for (int i = 0 ; i < index ; i++) {

            current = current->next_Flat ;

        }

        current->prev_Flat->next_Flat = new_flat ;
        new_flat->prev_Flat = current->prev_Flat ;
        current->prev_Flat = new_flat ;
        new_flat->next_Flat = current ;

    }

    delete new_flat;

}

FlatList b ;

b.add_flat(0,10,1) ;
cout << b.Head->id ;
Jason
  • 36,170
  • 5
  • 26
  • 60
Uygar Mutlu
  • 1
  • 1
    *"Should I define a destructor ? "* - yes you should – UnholySheep Nov 16 '22 at 16:56
  • does the line `Flat* current = Head ;` work ? where is `Head` declared? – Ivan Nov 16 '22 at 16:57
  • you destructor could be set by default, it depends on the class members – Ivan Nov 16 '22 at 16:58
  • 1
    do you **need** to use `new` ? can't you just create an instance of you object? – Ivan Nov 16 '22 at 16:59
  • So you create a new `Flat` and save its address into `new_flat`, `index` is 0 so you save its address into `Head`, then you delete `new_flat` (invalidating the address in `Head`), then with `cout << b.Head->id ;` you try to access that invalid address. Boom! – Fabio says Reinstate Monica Nov 16 '22 at 17:01
  • Try to minimize the use of new/delete (they are often not needed). For collection of objects there are containers like std::vector. And it looks like you could use something like [std::list](https://en.cppreference.com/w/cpp/container/list) here instead. And then your struct doesn't even need a next pointer and only needs to contain data (better separation of concerns too) – Pepijn Kramer Nov 16 '22 at 17:02
  • @Ivan Head is declared in the FlatList class and it works. I tried not to use new keyword but that didnt work. – Uygar Mutlu Nov 16 '22 at 17:02
  • 1
    It does not make sense to allocate a new node `new Flat()`, add it into the linked list, and then `delete` it at the end of the function. You delete the nodes when they are no longer in the list, not when they are still in the list. – john Nov 16 '22 at 17:03
  • @FabiosaysReinstateMonica Hmm I understood but so what is solution here Should I just not delete ? – Uygar Mutlu Nov 16 '22 at 17:05
  • @UygarMutlu In this function, you should not delete. Elsewhere in your program (like the destructor for instance) you should delete. – john Nov 16 '22 at 17:05
  • Observation: `add_flat` should reasonably leave you with one more `Flat` than you had before calling it. However, you create one (`new Flat()`) and destroy one (`delete new_flat`), so you still have as many as you started with. – molbdnilo Nov 16 '22 at 17:06
  • https://youtu.be/JfmTagWcqoE – Marek R Nov 16 '22 at 17:06
  • @john so if I delete the node in a different remove method is it ok and won't cause any leak ? – Uygar Mutlu Nov 16 '22 at 17:07
  • @UygarMutlu That sounds fine to me, but you should also write a destructor, to delete any nodes that are still left on the linked list when you are done with it. – john Nov 16 '22 at 17:08
  • If you delete nodes in a remove method, then the destructor can just call the remove method until the list is empty. – john Nov 16 '22 at 17:09
  • @john ok i will do that thanks for help in short notice. – Uygar Mutlu Nov 16 '22 at 17:10
  • Thank you guys all for the help I think I got it. Have a nice day everyone. – Uygar Mutlu Nov 16 '22 at 17:16

1 Answers1

0

Memory must be deallocated

  • when you are sure that the data it holds is no more needed,

  • before you drop all pointers that are referring to it.

If we take the case of a linked list, as long as you insert nodes, you must keep them. When you suppress one of the nodes, you can delete its allocated memory because you don't want this element anymore. Anyway, it is pointed to by the predecessor element or the list head. Hence you must redirect either the pointer of the preceding element or the list head to the node that follows.

Keeping pointer coherence requires great care as there can be tricky corner cases. If you forget to delete, you get a memory leak. If you forget to redirect some pointer, you get a dangling reference and a potential crash.