-1

Lets say in class A I have a pointer to a vector full of pointers to class B

Class A {
    ....
    std::vector<B *>* table;
    ....
}

Class B {
     int var1;
     int var2;
     B (const int _var1, const int _var2){
          var1 = _var1;
          var2 = _var2;
     }
}

How can I go about deleting table in class A in its destructor?

I tried

    ~A()
    {
        for (int i = 0; i < table->size(); i++)
        {
            delete (*table)[i];
        }
        delete[] table;
    }

but it is giving seg fault at the destructor for some reason.

Thanks for your help!

BlueXhusky
  • 1
  • 2
  • 4
    You probably didn't respect the [rule of 3/5/0](https://en.cppreference.com/w/cpp/language/rule_of_three) but we can't tell because you didn't provide a [MCVE]. Note that there is pretty much no reason for having an owning pointer to a vector (a pointer to a vector you need to make sure gets `delete`d). You probably just want `std::vector table;`. Overuse of pointers is a common mistake in c++. – François Andrieux Mar 12 '20 at 19:55
  • 1
    Why does `A` store a pointer to a vector in the first place? I think you should get rid of all the pointers in your example. – Henri Menke Mar 12 '20 at 19:55
  • You make sure you don't create a pointer to a vector of pointers in the first place. – n. m. could be an AI Mar 12 '20 at 19:55
  • I can see the problem with the pointer to pointer, let me see if I can simplify that implementation – BlueXhusky Mar 12 '20 at 19:58
  • 2
    @BlueXhusky rule of thumb - memory allocated with `new` must be deallocated with `delete`, and memory allocated with `new[]` must be deallocated with `delete[]`. Do not mix them. Better rule of thumb - in C++11 and later, don't use `new`/`delete` manually at all, use smart pointers instead (`std::unique_ptr` and `std::shared_ptr`) and let them handle all of the deallocation for you. – Remy Lebeau Mar 12 '20 at 20:02
  • You don't delete pointers, you delete what pointers *point to*. – Jesper Juhl Mar 12 '20 at 20:03
  • `delete (*table)[i];` -- How do you guarantee that what was placed in the vector was allocated with `new`? Issuing a `delete` on a pointer where you don't know where it came from is dangerous. – PaulMcKenzie Mar 12 '20 at 20:05

2 Answers2

2

Your segfault is caused by this line:

delete[] table;

which tells the compiler that you wish to delete an array. table is not an array, although it may act like it, it is a pointer to a single object on the heap and calling delete[] (instead of delete) therefore results in undefined behavior.

As a rule of thumb, never use naked pointers. If you do use naked pointers always use the same form for delete as you did for new.

As pointed out in comments, in modern C++ you should never have a container full of naked pointers (naked pointers are pointers which are not managed by a wrapper object using RAII).

You should therefore use something like std::unique_ptr<B> or std::shared_ptr<B> in place of B* in your vector. You should also not have a std::vector pointer and should instead copy or move (if copying is expensive) the vector into place. Use smart pointers instead of naked pointers and this hopefully wont happen again :p.

Object object
  • 1,939
  • 10
  • 19
  • Thank you for the smart pointer suggestion. I realized that copying don't have to be that expansive and managed to workout a solution that doesn't involve pointers as of now. – BlueXhusky Mar 12 '20 at 21:25
  • Ownership complicates things and is kinda out of the scope of this answer (haha), while naked pointers can be acceptable when you know the object it points too will live longer than it then theyre ok, but manually new'ing and delete'ing should _almost_ never be used (thats what I meant by 'use smart pointers') – Object object Mar 13 '20 at 09:23
-1

You are getting a Segmentation Fault because of delete[] table statement in your destructor.

Replace delete[] table; with just delete table;.

VHS
  • 9,534
  • 3
  • 19
  • 43
  • 1
    Using the wrong `delete` isn't the same as freeing memory that was already freed. Keep in mind that `delete` isn't `free`. `delete` does eventually cause memory to be freed but it also ends an object's lifetime and calls the destructor. – François Andrieux Mar 12 '20 at 20:05
  • And its actually not freeing already freed memory, its trying to delete memory which you dont own and/or is nothing to do with `table`. – Object object Mar 12 '20 at 20:09
  • @TheGoldKnight23 Can you please explain how you came to that conclusion? – François Andrieux Mar 12 '20 at 20:15
  • delete[] will delete the current pointer then look for the other elements of the array, even if they dont exist. Which means that when it is called on a single pointer it goes on a rampage of destruction as it never finds the bounds of the array and it is completely undefined as to what happens at that point – Object object Mar 12 '20 at 20:16
  • 2
    @TheGoldKnight23 That is just what *might* happen when you use the wrong `delete`. It's Undefined Behavior. No single possible outcome is any more valid than any other. It might work fine sometimes. It might not crash and corrupt a bunch of data. It might even crash before it reaches the `delete`. It's incorrect to presume how UB will manifest. – François Andrieux Mar 12 '20 at 20:18
  • Sure, but it will definitely cause a deletion of something (possibly many somethings) next to it on the heap and then we have objects of type X being destructed as objects of type A (fun :D). But the above segfault is not caused by freeing already freed memory but by using the wrong delete – Object object Mar 12 '20 at 20:22
  • @TheGoldKnight23 Yes, using the wrong `delete` is a problem that needs to be fixed. No, it won't necessarily delete something. It might not even delete `table` at all! You may be interested in reading about [time traveling Undefined Behavior](https://stackoverflow.com/questions/24527401/undefined-behavior-causing-time-travel). It's real. – François Andrieux Mar 12 '20 at 20:23
  • @FrançoisAndrieux `delete` indeed frees up memory pointed to by a pointer (and calls destructor). OP is using the delete array operator instead of the plain `delete`. – VHS Mar 12 '20 at 20:30
  • @VHS Of course, but the problem with using the wrong `delete` is that it is Undefined Behavior. You can't assume it will be a *memory* problem. `delete` is not `free`. It could just as well crash because it tried to execute a destructor with a bad `this`. I can fail for all sorts of reasons. Bringing in memory management is an error in this answer. – François Andrieux Mar 12 '20 at 20:31
  • @FrançoisAndrieux, I tried to reword my answer. I also read your link to the post about time traveling UB. It appears as if such 'time travel' may occur when the compiler sees a UB statically and optimizes it. However, in OP's case, a compiler may not be able to detect a UB and therefore, the `delete[] table` instruction would be executed at run time. – VHS Mar 13 '20 at 01:05
  • @VHS Maybe time traveling UB won't occur here but it's just an example to illustrate that UB can do *anything*. As a c++ developer you are required to not presume what UB will do. It's not because we can't think of a reason it would act a certain way that it never will. There could be any number of other reasons that `delete[]` isn't reached or that `delete[]` would not reach the point of freeing memory,. – François Andrieux Mar 13 '20 at 13:26