destructor and assignment operator

I have the following destructor and assignment operator

Code:

//destructor
Vehicle::~Vehicle( )
{
 if (s != NULL){
  	std::cout << "destructing " << s << std::endl;
	delete s;
 }
 else
	std::cout << "not destructing " << std::endl;	
}


// assignment operator
Vehicle& Vehicle::operator=( const Vehicle& other )
{
	if( &other != this )
	{
          s = other.s;
	   L_ = other.L_; 
          deltaS = other.deltaS;
	}
	
	return *this;
}

problem is it gives me an error of double free, how is this possible?? if s is already deleted previously s would be null and it wouldn't be deleted again... why is this?

You are just copying a pointer in your copy assignment operator. So, if the source object is destroyed, the object that its s points to is destroyed. It so happens that the destination object's s points to the same object, and its destructor tries to destroy that object again.

That's it.
You have to choose between copying the thing pointed at, or using a smart pointer to the item (or similar such behaviour), or using move semantics.
It's also usually a bad idea to test for self-assignment. See GotW for why
You also don't need to test for NULL prior to deletion.

Originally Posted by laserlight

You are just copying a pointer in your copy assignment operator. So, if the source object is destroyed, the object that its s points to is destroyed. It so happens that the destination object's s points to the same object, and its destructor tries to destroy that object again.

so then I should allocate a new memory first before on my assignment operator and not just assigning it?

Originally Posted by -EquinoX-

so then I should allocate a new memory first before on my assignment operator and not just assigning it?

The point is, if you have mutiple object responsible for deleting a single object then multiple delete's will invariably occur (you could instead use a pointer-to-a-pointer, which would allow you to delete, then nullify the shared pointer, but just keep in mind that you would need to also 'new' the pointed-to pointer as well). The best solution is a smart pointer, though (you might look into the boost for a good shared pointer implementation).

...you also don't need to test for NULL prior to deletion.

But you should set it to 0 ot NULL after deletion.

Originally Posted by Bubba

But you should set it to 0 ot NULL after deletion.

That is a good habbit, but then again it's also utterly pointless in a destructor. Still, it can't hurt...