Inserting a Smart Pointer in a Unordered Map calls destructor

Question

I'm making an engine and to handle materials stuff I have a static renderer class storing a static std::unordered_map<uint, Ref<Material>> m_Materials; (a material and its ID), being Ref the a shared pointer using the next methods:

template<typename T>
using Ref = std::shared_ptr<T>;

template<typename T, typename ... Args>
constexpr Ref<T> CreateRef(Args&& ... args) { return std::make_shared<T>(std::forward<Args>(args)...); }

template<typename T>
constexpr Ref<T> CreateRef(T* t) { return std::shared_ptr<T>(t); }

So a Material is a simple class constructed with an unsigned int (uint) to store its ID and a string to store its name, and to create them and store them, I have the next static function in the Renderer:

Ref<Material> Renderer::CreateMaterialWithID(uint material_id, const std::string& name)
{
    Ref<Material> mat = CreateRef<Material>(new Material(material_id, name));
    std::pair<uint, Ref<Material>> mat_pair(material_id, mat);

    m_Materials.insert(mat_pair);
    return mat;
}

The problem is that the insertion into the map, for some reason, calls the material destructor, resulting in the insertion of an empty smart pointer (though the ID is correctly inserted) [material_id, empty].

Now, the material and the pair created seems to properly store the data of the material, so I don't know why it is the map failing. I also tried to insert as initializer list ({material_id, mat}), to create the material as CreateRef<Material>(material_id, name) (without the new), and also to use emplace instead of insert but I have always the same result, and I don't understand why the map calls the destructor.

Anyone sees something wrong?

Offtopic: use `CreateRef` on constructor like this: `Foo::Foo(Logger* loger)`. — Marek R, Jul 18 '21 at 09:09
`The problem is that the insertion into the map, for some reason, calls the material destructor` maybe there is already some material with same `material_id`. In such case material is not updated and it `Material` is clean up. — Marek R, Jul 18 '21 at 09:16
I tried to reproduce but failed: [**Demo on coliru**](http://coliru.stacked-crooked.com/a/dd6e2525a8afb2d4). However, I must admit that I tested with `-std=c++17` where copy-elision is mandatory. I also noticed some strange effects in MSVC and Debug mode concerning this: [copy elision in MSVC & Debug](https://stackoverflow.com/questions/67623843/c-returning-object-from-a-function/67624435#comment119541850_67624435). — Scheff's Cat, Jul 18 '21 at 09:32
In the past (before I started with C++17), concerning maps, I sometimes ended up with `std::piecewise_construct` to the rescue. FYI: [SO: C++11 use-case for piecewise_construct of pair and tuple?](https://stackoverflow.com/q/6162201/7478597) — Scheff's Cat, Jul 18 '21 at 09:36
Hey I finally fixed it. The @MarekR comment made me think that what he said was impossible because I had a check to see if that material existed, but when I looked closer into it, I found that the first material inserted (the default one, through another function) was properly inserted, while the subsequents (through the function I posted) wasn't... So I discovered that the mentioned check was wrongly made and it was adding a key with an empty value and upon entering the posted function, the material id was already in the map with an empty value (making it through [] would have worked) — LuchoSuaya, Jul 18 '21 at 09:56
But that was my bad, I forget to also post the check function... I finally fixed the checking function anyway and it's now working perfectly :) — LuchoSuaya, Jul 18 '21 at 09:56
Also, @Scheff'sCat really thanks for your comments, they helped me a lot, I really appreciate when people link you to deeper C++ knowledge of this kind... Actually you made me check if in release config was working (it didn't, because the problem was another), but thanks — LuchoSuaya, Jul 18 '21 at 09:58
What it's still a mistery for me is why the destructor of material was being called... Probably because the insertion constructs it and then, upon checking it already exists a key, it destructs it? — LuchoSuaya, Jul 18 '21 at 09:59
_Probably because the insertion constructs it and then, upon checking it already exists a key, it destructs it?_ That's in fact possible: [**Updated Demo on coliru**](http://coliru.stacked-crooked.com/a/ee0865319a632e7f). — Scheff's Cat, Jul 18 '21 at 10:13

score 1 · Accepted Answer · answered Jul 19 '21 at 02:58

Try calling insert_or_assign; if the key already exists, insert will simply destroy the object you are trying to insert. As an example

m_Materials[0];
m_Materials.insert({0, std::make_shared<Material>(name)});

the shared_ptr will be discarded, because the first [0] created a nullptr value, and the insert later sees it and says "don't insert if it is already there".

m_Materials.insert_or_assign({0, std::make_shared<Material>(name)});

that replaces the nullptr stored there, as would

m_Materials[0]=std::make_shared<Material>(name);

As an aside, I'd be extremely leery of CreateRef having two overloads, one that takes constructor parameters the other a pointer. It is an example of confusing the map and the territory, which I find bites you in an uncomfortable place at annoying times.

Inserting a Smart Pointer in a Unordered Map calls destructor

1 Answers1