Boost C++ Libraries: Ticket #5146: Assertion fails in visitation_impl.hpp with nested variants

attachment set

Mon, 31 Jan 2011 22:08:18 GMT

attachment → variant_bug.cpp

Tue, 01 Feb 2011 19:33:54 GMT

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 — Fri, 11 Feb 2011 03:39:47 GMT

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

Antony Polukhin — Tue, 10 Dec 2013 15:02:54 GMT

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;
}

owner, status changed

Antony Polukhin — Tue, 10 Dec 2013 15:03:39 GMT

owner changed from ebf to Antony Polukhin
status new → assigned

status changed; resolution set

Antony Polukhin — Tue, 10 Dec 2013 15:05:03 GMT

status assigned → closed
resolution → invalid