Assertion fails in visitation_impl.hpp with nested variants

[Florian Goujeon <flo.goujeon@…>]

Let test_variant be following typedef:

typedef
	variant
	<
		int,
		ptr_to_test_variant
	>
	test_variant
;

ptr_to_test_variant is a simple class which contains a test_variant object allocated on the heap (to break the circular dependency, see attachment).

Here is the test case:

int main()
{
	test_variant tv1 = 88;
	test_variant ptr_to_tv1 = ptr_to_test_variant(tv1);
	test_variant ptr_to_ptr_to_tv1 = ptr_to_test_variant(ptr_to_tv1);

	const ptr_to_test_variant& direct_ptr_to_ptr_to_tv1 = get<ptr_to_test_variant>(ptr_to_ptr_to_tv1);
	ptr_to_ptr_to_tv1 = direct_ptr_to_ptr_to_tv1.pointed_test_variant();
}

Here is the output:

/usr/include/boost/variant/detail/visitation_impl.hpp:207: typename Visitor::result_type boost::detail::variant::visitation_impl(int, int, Visitor&, VPCV, mpl_::true_, NBF, W*, S*) [with W = mpl_::int_<20>, S = boost::detail::variant::visitation_impl_step<boost::mpl::l_iter<boost::mpl::l_end>, boost::mpl::l_iter<boost::mpl::l_end> >, Visitor = boost::variant<int, ptr_to_test_variant>::convert_copy_into, VPCV = void*, NBF = boost::variant<int, ptr_to_test_variant>::has_fallback_type_, typename Visitor::result_type = int, mpl_::true_ = mpl_::bool_<true>]: Assertion `false' failed.

I'm experiencing the exact same issue with my own implementation of stack-based variant (using variadic templates) as well. I'd like to know how to fix this…

[Florian Goujeon <flo.goujeon@…>]

OK, I got it.

Since ptr_to_ptr_to_tv1 kinda contains direct_ptr_to_ptr_to_tv1, variant has to destruct direct_ptr_to_ptr_to_tv1 AFTER direct_ptr_to_ptr_to_tv1.pointed_test_variant() was copied into ptr_to_ptr_to_tv1.

Currently, I guess variant clears its content (i.e. calls the destructor of its contained object) then sets it with a copy of the new object. This is the most natural behavior, but it causes problems when the copy constructor of the new object needs data that are stored in the previously contained object.

I don't know the code of Boost.Variant, so I won't be able to write a patch. But basically, the changes I made in my implementation can be summarized like this:

BEFORE:

template<typename Clear, typename Set>
void
clear_and_set(const Set& value)
{
	(*reinterpret_cast<Clear*>(buffer_)).~Clear(); //clear
	new(buffer_) Set(value); //set
}

AFTER:

template<typename Clear, typename Set>
void
clear_and_set(const Set& value)
{
	char old_buffer[Size];
	for(unsigned int i = 0; i < Size; ++i)
	{
		old_buffer[i] = buffer_[i];
	}

	new(buffer_) Set(value); //set

	(*reinterpret_cast<Clear*>(old_buffer)).~Clear(); //clear
}

[Steven Watanabe]

Please see The "Ideal" Solution: False Hopes for an explanation of why this solution is evil.

[Antony Polukhin]

This looks more like a ptr_to_test_variant issue, not variant's one.

Take a look at this code:

  const ptr_to_test_variant& direct_ptr_to_ptr_to_tv1 = get<ptr_to_test_variant>(ptr_to_ptr_to_tv1);
  ptr_to_ptr_to_tv1 = direct_ptr_to_ptr_to_tv1.pointed_test_variant();

You've got the chain: ptr_to_ptr_to_tv1 -> direct_ptr_to_ptr_to_tv1 -> unnamed

Then you try to do the following: ptr_to_ptr_to_tv1 = unnamed;

Before assigning anything to ptr_to_ptr_to_tv1 it will delete the data it owns:

delete direct_ptr_to_ptr_to_tv1;
delete unnamed;

And only after that will attempt to do the assignment, but delete unnamed is already called.

Much better solution would be to modify code of ptr_to_test_variant, (you do know that loops are possible, so make small trick to workaround such cases):

ptr_to_test_variant&
ptr_to_test_variant::operator=(const ptr_to_test_variant& rhs)
{
  test_variant* tmp_ptr = pointed_test_variant_;
  pointed_test_variant_ = new test_variant(*rhs.pointed_test_variant_);
  // Even if there was a loop, data is copied and new pointed_test_variant_
  // does not depends on old data at all. We are free to delete the old data.
  delete tmp_ptr;
  return *this;
}