Jump to content
  • Advertisement
Sign in to follow this  
D_Tr

copy constructor and assignment operator

This topic is 4156 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
Advertisement
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
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!