MultiIndex::insert(value_type&& x) destructs x even in case of the failure to insert

Question

Is it by design or a bug?

Normally I'd expect an insert to not destruct moved-in argument in case it failed to insert.

https://godbolt.org/z/bofeMo8fd

#include <memory>
#include <vector>

#include <boost/multi_index/hashed_index.hpp>
#include <boost/multi_index/identity.hpp>
#include <boost/multi_index/indexed_by.hpp>
#include <boost/multi_index/mem_fun.hpp>
#include <boost/multi_index/sequenced_index.hpp>
#include <boost/multi_index/tag.hpp>
#include <boost/multi_index_container.hpp>



class Base {
  public:
    virtual size_t GetId() const = 0;
    virtual ~Base() = default;
};

class Derived : public Base {
  public:
    size_t GetId() const { return 42; }
};


int main(int, char**) {
    // A tag to view elements in the order of insertion.
    struct Sequenced{};
    // A tag to view elements in the order sorted by type id.
    struct Ordered{};

    using ContainerType = boost::multi_index_container<
        // Elements are pointers to Base.
        std::unique_ptr<Base>,

        boost::multi_index::indexed_by<
            boost::multi_index::sequenced<boost::multi_index::tag<Sequenced>>,
            boost::multi_index::hashed_unique<
              boost::multi_index::tag<Ordered>,
              boost::multi_index::const_mem_fun<Base, size_t,
                                                &Base::GetId>>>>;

    ContainerType container;

    // Insert first element.
    auto& ordered_view = container.get<Ordered>();
    auto new_element = std::make_unique<Derived>();
    auto insert_result = ordered_view.insert(std::move(new_element));
    if (!insert_result.second) return -1;

    // Try toInsert second element with the same key.
    auto new_element2 = std::make_unique<Derived>();
    auto insert_result2 = ordered_view.insert(std::move(new_element2));
    assert(!insert_result2.second);
    assert(new_element2->GetId() == 42);
}

Results in segmentation fault

Answer 1

For the record, my answer as posted on the related issue on Github.

Boost.MultiIndex insertion does indeed not modify the value_type&& passed if insertion is unsuccessful. The problem with this line

ordered_view.insert(std::move(new_element))

is that insert expects a std::unique_ptr<Base>&& and you're passing a std::unique_ptr<Derived>&&: the way std::unique_ptr's implicit construction works, this creates a temporary std::unique_ptr<Base>&& to which the value of new_element is transferred. All of this happens before Boost.MultiIndex is even called, and the net result is that new_element will become empty regardless of whether insertion is successful or not.

One possible user-side workaround is to use something like this:

template<typename Container, typename Derived>
auto insert_unique_ptr(Container& c, std::unique_ptr<Derived>&& p)
{
  typename Container::value_type pb = std::move(p);
  auto res = c.insert(std::move(pb));
  p.reset(static_cast<Derived*>(pb.release()));
  return res;
}

(https://godbolt.org/z/P5KW6q585)

Answer 2

Note my answer missed the real cause of the problem. I'll keep the potentially informative bits about standard library behavior that you seemed to get have overly optimistic expectations about as well, because that may still be helpful information to others.

In fact, the corresponding standard library functions, like map::emplace/map::insert also may move even if insertion fails. I reckon this has to with trading off performance cost while maintaining strong exception safety guarantee (which is documented)

C++17 added new operations insert_or_assign and try_emplace specifically to add that guarantee:

Notes

Unlike insert or emplace, these functions do not move from rvalue arguments if the insertion does not happen, which makes it easy to manipulate maps whose values are move-only types, such as std::map<std::string, std::unique_ptr<foo>>. In addition, try_emplace treats the key and the arguments to the mapped_type separately, unlike emplace, which requires the arguments to construct a value_type (that is, a std::pair).