inserting temporary std::shared_ptr into std::map, is it bad?

Question

I'm designing a class for my application that implements a lot of standard shared pointers and usage of standard containers such as std::map and std::vector

It's very specific question to the problem so I just copied a piece of code from my header for clarification purposes.. here is a snapshot of that declarations from the header:

struct Drag;
std::map<short, std::shared_ptr<Drag>> m_drag;
typedef sigc::signal<void, Drag&> signal_bet;
inline signal_bet signal_right_top();

and here is one of the functions that uses the above declarations and a temporary shared_ptr which is intended to be used not only in this function but until some late time. that means after the function returns a shared pointer should be still alive because it will be assigned at some point to another shared_ptr.

void Table::Field::on_signal_left_top(Drag& drag)
{
    m_drag.insert(std::make_pair(drag.id, std::make_shared<Drag>(this))); // THIS!
    auto iter = m_drag.find(drag.id);
    *iter->second = drag;
    iter->second->cx = 0 - iter->second->tx;
    iter->second->cy = 0 - iter->second->ty;

    invalidate_window();
}

the above function first insert a new shared_ptr and then assigns the values from one object into another,

What I need from your answer is to tell whether is it safe to insert temporary shared_ptr into the map and be sure that it will not be a dangling or what ever bad thing.

According to THIS website the above function is not considered safe because it would much better to write it like so:

void Table::Field::on_signal_left_top(Drag& drag)
{
    std::shared_ptr pointer = std::make_shared<Drag>(this);
    m_drag.insert(std::make_pair(drag.id, pointer));
    auto iter = m_drag.find(drag.id);
    *iter->second = drag;
    // etc...
 }

well one line more in the function.

is it really required to type it like that and why ?

Answer 1

There's no difference between the two functions in regard to the std::shared_ptr, because the std::make_pair function will create a copy of the temporary object before the temporary object is destructed. That copy will in turn be copied into the std::map, and will then itself be destructed, leaving you with a copy-of-a-copy in the map. But because the two other objects have been destructed, the reference count of the object in the map will still be one.

---

As for handling the return value from insert, it's very simple:

auto result = m_drag.insert(...);
if (!result.second)
{
    std::cerr << "Could not insert value\n";
    return;
}

auto iter = result.first;

...

Answer 2

The code in the example given is different from your example code, because it is using the new operator instead of std::make_shared. The key part of their advice is here:

Since function arguments are evaluated in unspecified order, it is possible for new int(2) to be evaluated first, g() second, and we may never get to the shared_ptr constructor if g throws an exception.

std::make_shared eliminates this problem - any dynamic memory allocated while constructing an object within std::make_shared will be de-allocated if anything throws. You won't need to worry about temporary std::shared_ptrs in this case.