rakhimov / scram

Probabilistic Risk Analysis Tool (fault tree analysis, event tree analysis, etc.)
https://scram-pra.org
GNU General Public License v3.0
133 stars 54 forks source link

An alternative implementation of ext::extract #230

Closed joaquintides closed 6 years ago

joaquintides commented 6 years ago

As per this discussion, you might want to consider this implementation, which does not use const_cast:

template <typename T, typename... Ts>
T extract(typename boost::multi_index_container<T, Ts...>::iterator it,
          boost::multi_index_container<T, Ts...>* container) noexcept {
  assert(it != container->end());
  boost::optional<T> result;
  if(container->modify(it, [&](T& x){result = std::move(x);}))container->erase(it);
  return std::move(*result);
}
rakhimov commented 6 years ago

Thanks @joaquintides !

I have some comments though for your implementation.

First, you do not need optional, if it != container.end(). Second, I believe, the multi-index container will crash upon modification in your implementation because the value has been moved from. For example, if I have unique_ptr with object name() member function value as a key. After moving the object out of the container, multi-index will call the name() on nullptr to restore its internal invariants.

What I did is that I altogether broke multi-index contract of modification outside of the container, and simply result = std::move(*it). Apparently (and consisted with other std containers), upon erase, multi-index does not require access to the object pointed by the iterator, so erase(it) does not crash even though the object is gone.

joaquintides commented 6 years ago

First, you do not need optional, if it != container.end().

optional is used to cope with non default constructible Ts.

Second, I believe, the multi-index container will crash upon modification in your implementation because the value has been moved from.

The code assumes that a moved-from value remains valid (even if empty) --this is what happens with e.g. std::string. If this is not the case then I agree with you a crash will ensue.

rakhimov commented 6 years ago

optional is used to cope with non default constructible Ts.

Nice, I learned something today!

Second, I believe, the multi-index container will crash upon modification in your implementation
because the value has been moved from.

The code assumes that a moved-from value remains valid (even if empty) --this is what happens with e.g. std::string. If this is not the case then I agree with you a crash will ensue.

I once tried solving this with a "special" transferable pointer which kept track of ownership separate from the pointer value, but after discussions on reddit, my idea seems flawed.