CODEWITHSUNDEEP
c++oopshared-ptr
Sherlock Holmes
Sherlock Holmes

Reputation: 291

Scalar deleting error when deleting std::shared_ptr

I am trying to find out how can I delete children of my Block class. I tried to do it with raw pointer. I don't know why, but it didn't work. I was getting the scalar deleting error. I now tried to do this with std::shared_ptr. I t didn't work as well. I am removing the children with:

void Block::remove(Block* block)
    {
        std::shared_ptr<Block> ptr(block);
        auto it = std::find(children.begin(), children.end(), ptr);

        if (it != children.end())
        {
            *it = NULL;
            children.erase(it);
        }
    }

and the Block deleter is:

Block::~Block()
    {
        for (auto& child : this->children)
        {
            child = NULL;
        }


        if (!this->children.empty())
            this->children.clear();
    }

According to the debugging process, the ptr variable is found and then deleted. At the point of deleting everything works well until the last line, where I am getting the scalar deletion error. Just for the record: the children variable is of type std::vector<std::shared_ptr<Block>>.

EDIT: The full code is here: https://github.com/DragonGamesStudios/Ages-of-Life. All the block functions are defined in AOLGuiLibrary/source/Block.cpp

Upvotes: 0

Views: 1488

Answers (3)

Swift - Friday Pie
Swift - Friday Pie

Reputation: 14589

If block points at array code, this code is illegal, because for a new[] you have to call delete[], and std::shared_ptr<Block> would call delete. After that heap would be corrupted and further operations may fail. Same happens if blocks points to an element of array. If blocks points to object not located in heap, it also would result in error.

In general it's bad idea to delete a pointer passed to a function. There is no guarantee that the pointer was one returned by new, and even if it was: by which new?

To create a strong reference counter for an array, you have to use std::shared_ptr<Block[]>, but usually it's better idea to use some kind of container.

To find a value of pointer object in array of shared pointers:

auto it =  std::find_if( children.begin(), children.end(), [=](auto& el) { 
                return el.get() == block; 
           });

If childrenis not static (that's not some kind of factory), then that destructor code is completely redundant. After call to ~Block() there will be calls to destructors of every member of Block, including that collection which would deallocate its resources and call destructor to every pointer, which would call destructor to every Block.

Upvotes: 0

Chris Uzdavinis
Chris Uzdavinis

Reputation: 6131

Once a pointer has been given to a shared_ptr, that shared_ptr owns it. You must never again give that pointer to another shared pointer.

int * ptr = new int(1234);
{
    std::shared_ptr<int> shp1(ptr); // shp1 owns ptr
    std::shared_ptr<int> shp2(ptr); // shp2 owns ptr ???
}
// at this point ptr has been deleted twice

Note: it's preferable to not use new but to call std::make_shared and never give raw pointers to shared_ptr in the first place.

What you are doing to search for a child is exactly this problem. Passing in a raw pointer, you create a new owner for it. If that pointer is a child, it's already owned. You must not create shared pointers from raw pointers after they have been given to a shared pointer.

Also, the destructor you showed for Block isn't wrong, but it is completely unnecessary. All of what you coded there would happen automatically. Block is destroyed, it destroys the vector it holds (so clearing it is unnecessary.) The vector destroys its elements, so the shared pointers it holds are cleaned up too.

Upvotes: 0

Dmitry Kuzminov
Dmitry Kuzminov

Reputation: 6584

You are mixing shared_ptrs with raw pointers, and that is a bad practice. If I understand correctly, you store the blocks in the vector of shared pointers, and that means that this vector has an ownership on these objects. When you create an additional shared_ptr out of a raw pointer you create another object that has the ownership on the same object:

std::shared_ptr<Block> ptr(block);

As the result, you would try to delete the object twice, and that leads to the undefined behavior.

First of all: do you need shared pointers? Consider using unique_ptr as a less error prone idea (if only one pointer object has the ownership, it is easier to understand who and when would destroy the underlying object).

Next, NEVER use raw pointers in this context: you may use raw pointers only when you don't store/delete the object. And for sure don't create smart pointers out of something that is already stored in one of them already.

Upvotes: 2

Related Questions