Will this completely clean up the result of dynamic allocation of a class object?

Question

So I dynamically allocated a class object with this code:

void Inventory::createNewInventoryItem(std::string itemName, unsigned int maxQuantity, unsigned int orderThreshold, double price)
{
    InventoryItems* newItem = new InventoryItems;
    newItem->createInventoryItem(itemName, maxQuantity, orderThreshold, price);
    m_inventory.push_back(newItem);
}

Which is stored in this vector: std::vector m_inventory;

Then once the object is done being used deleteInventoryItem is called, which is defined as:

void Inventory::deleteInventoryItem(int posInVector)
{
    m_inventory.at(posInVector)->~InventoryItems();
    m_inventory.erase(m_inventory.begin() + posInVector);
}

My question is: Will this deleteInventoryItem function successfully free up the memory used by the dynamic allocation in createNewInventoryItem? (This ties into my confusion around if delete[] is needed when a class deconstructor is called)

Sorry for the not "Mission critical" question, just trying to make sure I'm not continuing bad practices as I'm really trying to program as perfectly as I could...

user_9593596 · Answer

Will this deleteInventoryItem function successfully free up the memory used by the dynamic allocation in createNewInventoryItem?

No, your code does leak memory. What you do here,

m_inventory.at(posInVector)->~InventoryItems();

is explicitly calling the destructor of the container element. This does clean up any resources held by the specific object, but not the memory resource that hosts the object itself. It is very, very rare that you have to manually call the destructor of an object (the only situation I can think of is using placement new, and you very rarely need that).

What you could have done is using delete to both clean up the memory and invoke the destructor of the element being erased. But as a much safer alternative for this manual memory management, I would advise you to choose one the following two options

If the object that is supposed to be stored in the container fine with being copied by value (i.e. not a polymorphic type and you want to store base class references), just store it by value:
```
std::vector myData;
```
Otherwise, store a std::unique_ptr which takes care of all the memory managent for you:
```
std::vector> myData;
```

user_2932052 · Answer

No it will memory-leak. You are obviously calling the destructor but do not call delete which is the counterpart of new. The new followed by a type allocates memory first then calls implicitly the destructor. The delete operator also does two things, first calling the destructor, then deallocate the memory.

There is rarely a need for explicitly calling the destructor, specifically for cleaning up instances created through the new placement syntax., and not calling ~ it in this case will lead to leaks if the instance contains hold dynamically bound resources via members or base class(es).

As a rule of thumb, keep these pairs in mind:

obj = new Class; needs delete obj;
obj = new (buffer) Class needs obj->~Class();
array = new Class[count]; needs delete [] array;

... and, as the most important in the collection, automatic objects:

Class obj; needs (nothing) ^{but the end of the enclosing scope}

The last "pair" shows an implicit call to (default) constructor (it could be any other constructor provided by the Class definition) and ends (at the closing brace) with an implicit call of the destructor, which, by the way, the reason for the C++ language to limit the destructors of a class to at maximum one: The compiler depends on this uniqueness to be able to automatically close scopes. This very automatism also forms the basis for std::unique_ptr and std::shared_ptr.

Will this completely clean up the result of dynamic allocation of a class object?

2 Answers