0

I am concerned about creating and then pushing the dynamic objects into vector of pointers. I would like to know how to implement this correctly to avoid any kind of memory leaks. Here's what I have:

//THIS CREATES VECTOR
vector <combustion_car*> combustion_car_list;
//THIS IS CREATING AND PUSHING THE OBJECT
combustion_car* temporary=new combustion_car;
                temporary->create();
                combustion_car_list.push_back(temporary);
                cout<<"Object added"<<endl;
                temporary=NULL;
                delete temporary;
//THIS DELETES OBCJET FROM VECTOR
combustion_car_list.erase(combustion_car_list.begin() + (choice-1));
Marcel Buczkowski
  • 3
  • 2
  • 3
    If you don't need to use pointers then don't. If you do, then use a smart pointer and avoid manual memory management. – Retired Ninja Jun 09 '20 at 00:32
  • 1
    In C++ use `nullptr` in preference to `NULL`. Also *never* `delete` a null pointer. – tadman Jun 09 '20 at 00:36
  • You should consider skipping the pointer part and just use [`emplace_back`](https://en.cppreference.com/w/cpp/container/vector/emplace_back). – tadman Jun 09 '20 at 00:37
  • if you do manual memory management, first use delete statement and the assign nullptr or NULL to your temporary object. – Aneury Perez Jun 09 '20 at 00:40
  • @AneuryPerez The problem is it's been pushed into the vector, so that invalidates that pointer. Best to just leave it. – tadman Jun 09 '20 at 00:50
  • That `erase` does *not* delete objects. It deletes *pointers* and abandons the objects. That is a memory leak, plain and simple. – Beta Jun 09 '20 at 00:53
  • Unrelated: `temporary->create();` seems like a bad idea. Except for relatively uncommon cases where two-phase construction is advantageous or necessary, the constructor should do all of the creating. – user4581301 Jun 09 '20 at 00:53
  • @RetiredNinja Yes, I do need to use pointers. This code is only a small part of my project. So If I use a smart pointer I dont have to worry about cleaning up my memory and while creating an object I can simply end my code on line cout<<"object added"< – Marcel Buczkowski Jun 09 '20 at 00:53
  • @user4581301 my class is multiply inherited and has a lot of variables that have to be manually set, that's why I use create(); instead of constructor. – Marcel Buczkowski Jun 09 '20 at 00:57
  • 1
    @tadman "*Also never delete a null pointer*" - why would you say that? A null pointer is *perfectly safe* to call `delete` on. – Remy Lebeau Jun 09 '20 at 02:05
  • @MarcelBuczkowski you should not be `delete`'ing a pointer you just `push_back()`'ed into a container. That leaves the container holding a **dangling pointer to invalid memory**, which *will* crash your code later on when you try to use that object you `delete`'d. – Remy Lebeau Jun 09 '20 at 02:07
  • @RemyLebeau It's an utterly pointless thing to do when you know it's null, as in this code. Perhaps I should've been more specific as you point out. – tadman Jun 09 '20 at 03:31
  • @tadman `delete` checks for null for you, so doing the same check manually is redundant. – Remy Lebeau Jun 09 '20 at 05:24
  • @RemyLebeau Oh, I mean when the line above is literally `ptr = nullptr` then `delete ptr` is just plain pointless. – tadman Jun 09 '20 at 06:00
  • @tadman obviously doing `temporary=NULL; delete temporary;` is backwards, it should be `delete temporary; temporary=NULL;` instead. – Remy Lebeau Jun 09 '20 at 06:10

1 Answers1

0

Taking your original code sample, following are some suggested modifications to make it safely avoid memory leaks.

  1. Rather than use a std::vector of bare pointers, use a collection of std::unique_ptr objects:

    //THIS CREATES VECTOR
std::vector<std::unique_ptr<combustion_car>> combustion_car_list;

  2. Create your temporary object using a call to std::make_unique, and transfer its ownership to the container using std::move:

    //THIS IS CREATING AND PUSHING THE OBJECT
std::unique_ptr<combustion_car> temporary=std::make_unique<combustion_car>();
temporary->create();
combustion_car_list.push_back(std::move(temporary));
std::cout<<"Object added"<<std::endl;

  3. Remove the element at combustion_car_list[choice] (choice-1 is incorrect, but also dangerous because it could be 0, yielding an iterator value that is before combustion_car_list.begin()):

    //THIS DELETES OBJECT FROM VECTOR
combustion_car_list.erase(combustion_car_list.begin() + choice);

You'll also notice explicit use of std namespace throughout; see: Why is “using namespace std;” considered bad practice?

Phil Brubaker
  • 1,257
  • 3
  • 11
  • 14