3

I had a vector<Points*> points; (size: 6 with all unique Points) in my program wherein I was iterating through the points to draw something on the screen. However, as per my new requirements, I'd to increase the length of the vector to size: 14.

The new items that were to be added had to be from the previous 6 Points, so instead of allocating new memory, I thought of just using the previous pointers as follows:

while (currentSize < 14){
  int rndPoint = getRandomPoint(0, 5); //random index to choose from the vector
  points->push_back(points[randPoint]);
}

In the destructor of the class, when I've to deallocate the memory, I'm doing the following:

for(int i=0;i<points.size(); ++i){
  if(points[i] != NULL){
    delete (points[i]);
  }
}

However, when I try to exit the program - I'm getting an access violation error in the loop (specifically when i reaches index 6). When I've already deleted the 6 unique points by using delete, why is the condition if (points[i] != NULL) resulting in true for i=6,7...13?

Niall
  • 30,036
  • 10
  • 99
  • 142
user1240679
  • 6,829
  • 17
  • 60
  • 89
  • Set `points[i] = NULL;` after the `delete`, that's not done automatically. Also check if your class complies the [rule of three](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three). – πάντα ῥεῖ Aug 31 '14 at 18:19
  • @πάνταῥεῖ That’s not what the question is about. – Jonas Schäfer Aug 31 '14 at 18:20
  • @πάνταῥεῖ That's not sufficient. Some of the elements `points[]` are the same as the earlier ones, ergo double delete. – T.C. Aug 31 '14 at 18:21
  • @πάνταῥεῖ : wow! really?! so, in practice, every time a pointer is deleted is should also be set to `NULL`, especially in such a case where there might be two pointers pointing to the same thing. – user1240679 Aug 31 '14 at 18:21
  • @user1240679 That's what [`std::shared_ptr`](http://en.cppreference.com/w/cpp/memory/shared_ptr) is for. – πάντα ῥεῖ Aug 31 '14 at 18:23
  • Honestly, I'm unsure why you want to use pointers and `new` for something that sounds so simple. What's wrong with `std::vector`? – T.C. Aug 31 '14 at 18:25
  • @T.C : The problem above is actually too simplied. I'm doing the same in a different context where instead of `Point`, I've a large data structure. – user1240679 Aug 31 '14 at 18:27
  • 1
    @user1240679 Then the best recommendation really would be to use a `std::vector>` instead or managing raw pointers yourself. – πάντα ῥεῖ Aug 31 '14 at 18:28

3 Answers3

7

Use a smart pointer. If your program's source contains delete, and it is not in a deleter for a smart pointer, your program is broken. (And why the hell would you ever do that instead of std::default_deleter?).

The winner of the "Best Smart Pointer Award 2014" is std::unique_ptr, with an honorable mention for std::shared_ptr for those times when you've just gotta copy the pointer.

The Rule of Zero means that you should hardly ever need to implement a destructor/etc yourself, since you should always be using generic resource-managing classes to manage it yourself.

Puppy
  • 144,682
  • 38
  • 256
  • 465
  • 1
    Not to mention the combination of `std::shared_ptr` and [`std::weak_ptr`](http://en.cppreference.com/w/cpp/memory/weak_ptr), which looks quite appropriate to manage the use case the OP explained. – πάντα ῥεῖ Aug 31 '14 at 18:48
  • 1
    Not really found `weak_ptr` to be all that useful myself. If you're gonna pay for refcounting to enable copying, then you may as well hold a `shared_ptr` the vast majority of the time. – Puppy Aug 31 '14 at 18:50
4

The reason is that by deletion, other references to the same memory are not set to nullptr (or 0, pre C++11). Consider:

int *foo = new int;
// foo should be non-nullptr now
int *bar = foo;
// bar should be non-nullptr now
delete foo;
// both foo and bar are still non-nullptr.

There are three ways to solve your specific problem:

  1. Use std::shared_ptr

  2. Have a separate std::vector for the unique instances:

    std::vector<Point> unique_points;
std::vector<Point*> used_points;

// create all needed points *once* in unique_points
// insert pointers to the points in unique_points into used_points

  3. Just make copies.

Jonas Schäfer
  • 20,140
  • 5
  • 55
  • 69
  • Why wouldn't the answer below of @Skaktal work by setting the items to NULL after deleting? Just had to as because I didn't see that in the options you gave above. – user1240679 Aug 31 '14 at 18:26
  • @user1240679 That doesn't set other pointers to the same object to null. – T.C. Aug 31 '14 at 18:28
  • Aha! So, will such an assignment be valid `vector* pointsList`, `pointsList = sharedPointsList` where `sharedPointsList` is `vector*`. Also, what would be the best way to make copies of the data to which the pointer is pointing to in the above case? – user1240679 Aug 31 '14 at 18:34
  • 1
    ``new Point(*other_point)`` makes a new object which is a copy of the other (assuming that you have proper copy-constructors in place) – Jonas Schäfer Aug 31 '14 at 18:48
0

The generic answer is, when you use dynamic memory, have a PLAN for cleaning up.

One plan is to use the various forms of standard library template pointer managers (as describe above). However, there are other ways to do this as well.

The "best" plan depends upon the type of application at hand. For example, when you are using some graph structures reference counting may not work when that method works find with other types of structures.

For example if you try to do reference counting when you have

A points to B.
B points to C and D
C and D points to E
E points to B

That gives the following reference counts

B 2 C 1 D 1 E 2

Destroying the link A->B gives these reference counts

B 1 C 1 D 1 E 2

with the no reference to the structure, so it will never be deleted.

Pick the best plan for your situation.

user3344003
  • 20,574
  • 3
  • 26
  • 62