• Advertisement
Sign in to follow this  

C++ assignment operator

This topic is 3320 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

In the current C++ standard there are no move semantics, so with respect to the assignment operator in Factory: is the following safe? if it is not safe why and how would it be done? does it indicate a design flaw or just a language flaw?
 
class foo{...};
class bar{...};
class baz
{
	foo& foo_;
	bar& bar_;
public:
 	bar(foo& f, bar& b):foo_(f),bar_(b){}
 	...
};

class Factory
{
	foo& foo_;
	bar& bar_;
public:
	Factory(foo& f,bar& b):foo_(f),bar(b){}
	Factory(Factory const& rhs):foo_(rhs.foo_),bar(rhs.bar_){}
	Factory& operator=(Factory const& rhs)
	{
		if(this != &rhs)
		{
				this->~Factory();
				new (this) Factory(rhs);
		}
		return *this;
	}
	baz create_baz(){ return baz(foo_,bar_); }
	...
};



Thanks

Share this post


Link to post
Share on other sites
Advertisement
class MyDerivedClass : public Factory { ... };

MyDerivedClass a, b;
a = b; // Whoops, now "a" is a Factory instance ...

Share this post


Link to post
Share on other sites
Yes I never thought about slicing, but if this were an object that was not to have derived instances would it be ok?

I should also point out the this is just demo code and the real code would return shared pointers to base classes when a create method is called.

Share this post


Link to post
Share on other sites
I think it would be cleaner if instead of destructing and re-newing "this", you instead destructed and re-newed a member variable (so that said member variable would not have to be aware that it's being used that way, while the containing object would behave like any other objec)t. A bit like boost::optional but without the optional part (perhaps a boost::variant with a single type).

Share this post


Link to post
Share on other sites
That's funny, I was literally just reading Item 41 in Herb Sutter's Exceptional C++, which presents this exact same question, and a note saying that it appears on various newsgroups every few months, which I now believe :)

Basically although this sometimes works in some situations and more or less achieves what you're trying to do, don't do it because it's "exceedingly poor form and causes no end of problems". In summary, it can slice objects, it's not exception-safe, and it changes normal object lifetimes.

Prefer to write copy assignment in terms of copy construction, like this:


T& T::operator=(const T& other)
{
T temp(other); // do all the work off to the side
Swap(temp); // then 'commit' the work using only nonthrowing operations
return *this;
}




Where 'Swap' is a nonthrowing member function that swaps the guts of two objects. Have a read of either Effective C++ or Exceptional C++ for more details.

Hope that helps :)

Share this post


Link to post
Share on other sites
Quote:
I think it would be cleaner if instead of destructing and re-newing "this", you instead destructed and re-newed a member variable

Yes but the member variable is a reference and you can not reseat a reference and the reference should be the same for all instances created via the factory and between factories.

Quote:
In summary, it can slice objects, it's not exception-safe, and it changes normal object lifetimes.

If the copy constructor was declared explicit then no implicit conversion would happen and therefore it would be an error to try and copy the instance. yes?
Quote:
Where 'Swap' is a nonthrowing member function that swaps the guts of two objects.
How would a copy function be implemented with references?

Share this post


Link to post
Share on other sites
Quote:
Original post by CmpDev
Yes but the member variable is a reference and you can not reseat a reference and the reference should be the same for all instances created via the factory and between factories.
What I meant:

class Functionality
{ /* Look ma, I have a reference member ...
... and no assignment operator. */
};

class Wrapper
{ /* Implements assignment by re-allocating
the "data" member. */

Functionality data;
public:
operator Functionality&() {}
operator const Functionality&() const {} };

Share this post


Link to post
Share on other sites
ToohrVyk If I understand you correctly your solution is like the pimpl idiom, which that instance actually stores the references yes?

Share this post


Link to post
Share on other sites
Quote:
Original post by CmpDev
ToohrVyk If I understand you correctly your solution is like the pimpl idiom, which that instance actually stores the references yes?
Close (the pimpl idiom uses a pointer, so that the definition of the internal implementation doesn't have to be known by other translation units), but yes.

Share this post


Link to post
Share on other sites
Thank you that seems the most elegant solution without being able to use move.

Share this post


Link to post
Share on other sites
Incidentally, Swap is very easily implemented when you're using pimpl, and conversely a good reason for using pimpl is so you can write Swap in a simple, nonthrowing way.

Ignoring pimpl though, why not use smart pointers instead of references here?

Share this post


Link to post
Share on other sites
Quote:
this->~Factory();
new (this) Factory(rhs);


One reason that kind of thing is not done is that a constructor may throw.
Here, it doesn't so there is no problem.

However, one other problem I believe is that you're not allowed to destruct an object from within one of its member functions. You would need to check the standard to know for sure. There are simple workarounds though, just make a wrapper around an assignment-less Factory and add the operator= there.

The original problem, though, seems to be you need to use resettable references. Since pointers are evil, I recommend you use reference_wrapper from TR1 or Boost.

Also, the recommended idiom to write assignment operators is not
Quote:
T& T::operator=(const T& other)
{
T temp(other); // do all the work off to the side
Swap(temp); // then 'commit' the work using only nonthrowing operations
return *this;
}

but
T& T::operator=(T other)
{
using std::swap;
::swap(other, *this);
return *this;
}

Share this post


Link to post
Share on other sites
I prefer that form too, I was just typing up from Exceptional C++ which is right next to me! The important difference is to use ::swap, which could be user-supplied or std::swap.

I do prefer making the temporary copy obvious though; saving one line of code to hide it in a pass-by-value isn't big and it isn't clever.

Share this post


Link to post
Share on other sites
Quote:
Original post by loufoque
T& T::operator=(T other)
{
using std::swap;
::swap(other, *this);
return *this;
}


Why not a const reference as the parameter?

Share this post


Link to post
Share on other sites
Quote:
Original post by phresnel
Quote:
Original post by loufoque
T& T::operator=(T other)
{
using std::swap;
::swap(other, *this);
return *this;
}


Why not a const reference as the parameter?


Because you have to make the copy anyway if you want rollback semantics.

Share this post


Link to post
Share on other sites
Quote:
T& T::operator=(T other)
{
using std::swap;
::swap(other, *this);
return *this;
}


std::swap calls the assignment operator, so this is just going to result in a stack overflow.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement