Sign in to follow this  

copy constructor and assignment operator

This topic is 3850 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

Hello ppl, i 'd like to ask about the correctness of the assignment operator and copy constructor defined in the following piece of code. I have written a small test for self-assignment and it think it's correct but i want to be sure. Is there any case i 'm missing?

namespace Map2D
{
namespace Color
{

class UIntRGBA8
{
public:
	struct Element
	{
		UInt8 R;
		UInt8 G;
		UInt8 B;
		UInt8 A;
				
		Element(){};
				
		Element(UInt8 _R, UInt8 _G, UInt8 _B, UInt8 _A)
		{
			R = _R;
			G = _G;
			B = _B;
			A = _A;
		};
	};
			
private:				
	UInt32 width;
	UInt32 height;
			
	Element* pixels;
	
public:
	UIntRGBA8(UInt32 _width, UInt32 _height)
	{
		width = _width;
		height = _height;
		pixels = new Element[width * height];
	};
			
	UIntRGBA8(const UIntRGBA8 &_UIntRGBA8)
	{
		width = _UIntRGBA8.width;
		height = _UIntRGBA8.height;
		pixels = new Element[width * height];
		memcpy(pixels, _UIntRGBA8.pixels, sizeof(Element) * width * height);
	};
	
	UIntRGBA8& operator=(const UIntRGBA8 &_UIntRGBA8)
	{
		width = _UIntRGBA8.width;
		height = _UIntRGBA8.height;
		Element *tmp = new Element[_UIntRGBA8.width * _UIntRGBA8.height];
		memcpy(tmp, _UIntRGBA8.pixels, sizeof(Element) * width * height);
		delete[] pixels;
		pixels = tmp;
		return *this;
	};
			
	~UIntRGBA8()
	{
		delete[] pixels;
	};
};

};

};




Share this post


Link to post
Share on other sites
Its correct in that it will work "normally" but its not exception safe. In operator=() if the allocation fails (i.e. new throws) then your object will be left in an inconsistent state.

Some points:
1) You don't have to do all that if you use std::vector<>
2) You shouldn't be using memcpy in c++, you should use std::copy instead
3) In general you should avoid names with a leading underscore as the c++ standard reserves them for the implementation in the global and std namespaces.

Share this post


Link to post
Share on other sites
Thx for the advice Julian!

I used to use the 'Src' suffix for my constructors' arguments but for some reason i changed it to underscore... I guess i 'll change it back :P

I 'll consider using the vector class, though i 'd first like to get the existing code to be correct.

If i change this:

UIntRGBA8& operator=(const UIntRGBA8 &_UIntRGBA8)
{
width = _UIntRGBA8.width;
height = _UIntRGBA8.height;
Element *tmp = new Element[_UIntRGBA8.width * _UIntRGBA8.height];
memcpy(tmp, _UIntRGBA8.pixels, sizeof(Element) * width * height);
delete[] pixels;
pixels = tmp;
return *this;
};



to this:

UIntRGBA8& operator=(const UIntRGBA8 &_UIntRGBA8)
{
Element *tmp = new Element[_UIntRGBA8.width * _UIntRGBA8.height];
width = _UIntRGBA8.width;
height = _UIntRGBA8.height;
memcpy(tmp, _UIntRGBA8.pixels, sizeof(Element) * width * height);
delete[] pixels;
pixels = tmp;
return *this;
};



So that new throws before i do any changes to the object, am i safe? Or should i use some kind of exception handling?

Share this post


Link to post
Share on other sites
you should always check for the special case when you say
something like

UIntRGBA8 x;
x = x;

UIntRGBA8& operator=(const UIntRGBA8 &_UIntRGBA8)
{
if(this == &_UIntRGBA8) return *this;
// rest of code
}

one thing i've always thought funny was that in VS,
this is valid code even though it don't make any sense ha ha ha:

struct Object { Object(const Object&) {} };
Object obj(obj); // make a copy of an object that hasn't been constructed yet?

Share this post


Link to post
Share on other sites
Quote:
I used to use the 'Src' suffix for my constructors' arguments but for some reason i changed it to underscore... I guess i 'll change it back :P


I'd consider why you actually need to put a suffix at all, for example the following is unambiguous:


struct foo
{
// I forgot to mention in the first post, you should use
// initializer lists wherever possible
foo(int value) : value(value) { }
private:
int value;
};




And if you need to write a body for the constructor then you can use this->value to access member variables.

Quote:

If i change this:

<snip>

So that new throws before i do any changes to the object, am i safe? Or should i use some kind of exception handling?


Yep, that should be safe.

Quote:
you should always check for the special case when you say
something like

UIntRGBA8 x;
x = x;


The allocation of a new block of memory means that as his operator= is currently written it will work correctly in this case without special handling (which is what I interpreted his original question as asking)

Quote:
struct Object { Object(const Object&) {} };
Object obj(obj); // make a copy of an object that hasn't been constructed yet?


IIRC this results in undefined behavior regardless of weather you have written the copy constructor to deal with it.

Share this post


Link to post
Share on other sites

This topic is 3850 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.

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this