I agree on the point with re-assigning the variable is basically using it for
any new purpose, including for a state of a different task. So I'm fine with
leaving everything as it is now.

Thanks for the explanation! I was confusing myself a bit there because I thought
that once we have moved from an object, its contents don't really matter any
more because it cannot be used further anyway. Objects that have been moved from
are officially in an undefined state, and almost all operations on them other
than destruction are illegal. So I was wondering if code like the one for swap
should be legal (where we assign to b after it has been moved from.)

But *assigning* to such a moved-from object is legal and returns it to a defined
state. I think assigning to it is the only thing that may legally be done with
such an object other than destructing it. Theoretically I should know this, as
I've read up on all that very recently. But I find it hard to keep all these
things in my brain at the same time.

So I see that our implementation (which cleared the task for the moved-from
object while not setting it for the assigned-to one) would be wrong even without
mixing tasks.

>> In any case, the old code was bad. If we wanted operator= from a different task
>> to be illegal, we should have guarded that with an assertion.
> Should I add such an assertion?

Not sure; I find assignment to a state from a different task less strange than
comparison between tasks because in some sense assignment repurposes the object
for something different, so it is a bit like a destruction + construction. I
think we probably don't need the assertion.

>> Is there some other operation we are perhaps not supporting correctly?
> Hard to answer "no" with this given that this bug has been undiscovered since
> the introduction of the State class, which was at least 2 years ago, rather 3 or
> even 4.

This move assignment operator was apparently added on July 2, 2015, but without
the assignment `other.task = nullptr`. That assignment was added when issue791
was merged, in September last year. So until then the swapping would have worked
with states that belong to the same task.

Anyway, the question was more along the lines of "Should we have a look at the
class again and check if there are other issues?". I had a brief look and didn't
see anything, so perhaps that's good enough for now.

> No, I see, the point is that the expectation previously was that states of
> different tasks must never be mixed in the same object.

The expectation still is that we don't mix states of different tasks. 

> I'm not sure how/why this could cause problems. Not overwriting the task (as in
> the old code) means keeping the task of the old object, which should be fine in
> this context.

Swap causes a problem because we move assign to an already invalidated variable.
Consider the following implementation of swap

template <class T> void swap (T& a, T& b)
{
  T c(std::move(a)); a=std::move(b); b=std::move(c);
}

and the example of swapping two objects of type State. States have two members:
a raw pointer task and a vector<int> values.

Move-constructing c from a is fine. Note that a.task now is nullptr and a.values
is empty. Thus, move-assigning b to a, not copying b.task into a.task, leaves
a.task a nullptr.

> In any case, the old code was bad. If we wanted operator= from a different task
> to be illegal, we should have guarded that with an assertion.
Should I add such an assertion?

> Is there some other operation we are perhaps not supporting correctly?
Hard to answer "no" with this given that this bug has been undiscovered since
the introduction of the State class, which was at least 2 years ago, rather 3 or
even 4.

No, I see, the point is that the expectation previously was that states of
different tasks must never be mixed in the same object. This is also related to
the design decision that operator== considers comparison between different tasks
an error rather than just returning false.

In any case, the old code was bad. If we wanted operator= from a different task
to be illegal, we should have guarded that with an assertion.

> Reverse calls swap which uses the move assignment operator,
> which thus led to very weird undefined behavior...

I'm not sure how/why this could cause problems. Not overwriting the task (as in
the old code) means keeping the task of the old object, which should be fine in
this context. Is there some other operation we are perhaps not supporting correctly?

Apparently yes. There is a move constructor, too, which was correct already, and
which probably was enough for all cases. 

I stumbled across this (spending quite a few hours) when trying to implement the
hill climbing search for CEGAR-PDBs, reversing the resulting vector<State> into
the right order. Reverse calls swap which uses the move assignment operator,
which thus led to very weird undefined behavior...

How could this possibly work? Was it unused?

Resolved and pushed.

The move assignment operator of the State class does not copy the raw pointer to
AbstractTask.