Skip to main content
GameDev.net gamedev.net
🔒 Locked

Operator Overloading Again :-(

Started by thejoshinator Feb 17, 2005 at 10:39 PM 10 replies 1.9k views
Original Post
thejoshinator
thejoshinator
Alright, I've finally got my overloaded = operator to work, except for one final problem: how can you check to see if the source object is valid? Here's the code:
CTile& CTile::operator =(CTile &src_tile)
{
	// This function overloads the = operator so we can copy tiles correctly; *this is the left-hand operand(dest)
	// and src_tile is the right-hand operand(src)

//****************************************************************************//
	// This works for the destination validation
        // Check if the left operand is valid
	if (!this)
		return *this; // it's not, so return the pointer

        // Seems like it should work, but compiler complains that the ! operator isn't defined for CTile. Should I define it and do it this way?
	// Check if the right operand is valid
	if (!src_tile)
		return *this; // it's not valid, so return the pointer

        // This compiles, but doesn't work...dunno why :-S
        if (!&src_tile)
                return *this; // it's not valid, so return the pointer

//contents of function...blah blah

	// return pointer to existing object
	return *this;
} // end CTile::operator=();

Any ideas? Thanks, The Joshinator
Wavarian
Wavarian
For starters, "this" can never be NULL. Also, a reference can never be NULL either. Thus your NULL checks are fruitless. If you want to see if src_tile is valid (ie, if it has specific properties), you might want to either define a method called IsValid, or just define the ! and () operators. Then you can do something like this:

if(src_tile.IsValid()) ...
Zahlman
Zahlman
A reference cannot validly be NULL. 'this' won't be null either (or else it's already too late to do anything about it), and even if it were, returning *this would be a pretty bad idea in that case, yes?

So. Don't do any of this checking. One of the reasons for using a reference is so you don't have to worry about it. You should worry about the possibility of self-assignment, but the copy-and-swap idiom takes care of that for you (and also is exception safe... and also means you never duplicate code from copy ctor to assignment op ever again):

CTile& CTile::operator =(CTile &src_tile) {  // Invoke copy ctor. You will need to write it anyway. :)  CTile result(src_tile);  // Swap the data from the copy into the current object (which is known to exist).  std::swap(result, *this);  return *this;  // At end of function, the copied object has its dtor called, which cleans up  // the data that used to exist in 'this' (since it got swapped into the copy).  // So everything works correctly here as long as the copy ctor and the dtor  // are correct.  // Meanwhile, if the copy ctor throws, the object does not get constructed,  // and you won't reach any code that changes 'this' - thus you are exception-  // safe as long as the copy ctor is. What you gained was separation of the  // logic that could throw (making copies of resources) from the logic that  // can't (assigning the resource copies to members of the current object).}
thejoshinator
thejoshinator
I disagree with the idea that *this can never be null. I have a function that returns a pointer to an object if one exists at a particular coordinate, and returns NULL if there is no object at that coordinate. Therefore, if I do this:

*GetPointerToObject() = anotherobject;

anotherobject is copied to the object at the address returned by GetPointerToObject(), unless GetPointerToObject() returns NULL, in which case *this is NULL because *this points to the object to which the data is being copied. I thought I could apply the same idea to the reference, but I guess not :-S. I did find a workaround, though, so it's all good now :-) Thanks for the help guys!
Qw3r7yU10p!
Qw3r7yU10p!
Quote:
Original post by thejoshinator
I disagree with the idea that *this can never be null.


If you dereference a null pointer you are entering the land of 'undefined behaviour'. It's not something you should ever do.

I'm not interested in how your compiler behaves if you do it because I don't use your compiler.
thejoshinator
thejoshinator
I'm sorry if I seem a little ignorant, but all I know about programming has come from message boards, books, and online tutorials because there's no programming ed here. Having said this, what do you mean when you say "undefined behavior?"
Deyja
Deyja
Undefined behavior means that what the compiler does is undefined. The compiler can generate code to do whatever it wants. Generally, undefined behavior means it works fine on your machine, and in all your tests, then blows up in the customers face.

Also; an operator= that must explicitly check for self-assignment to work properly is icky.
Xai
Xai
thejosh, sure a reference CAN be using a NULL address ... because someone can pass in a dereferenced null pointer ... but what we are telling you is that that is the CALLER's bug already, it is not for you to concern yourself with inside of a function which takes a reference. I mean, you don't try to make sure that the caller is properly reference counting do you, or that the caller hasn't already called delete on the object ... (the answer is no, partially because you plain and simply CANT do these things).

also, you should always use a const reference for copy constructors and assignment operators ... it just plain isn't right to modify the right argument to those operations.

MyClass a(b); // should never modify b
a = b; // should never modify b

hence use "const CTile &src_tile" as your argument.

Also people, do you really write in checks for source / dest assignment overlap?

such as ...

a = a; //????

or even being more realistic:

SomeType a = GetTheGlobalCoolObject();
CurrentObject() = (b < 100) ? a : GetThePreviousCoolObject();

what are the chances any of you have ever written code that returns such things as object references - not pointers (which would work) and not objects (copies, which would work too) ....

I've been involved at 4 companies doing C++ code, with many many bugs in it, thousands of bugs, and some really crappy software for some of it (and some cool advanced stuff too) - and I've never once seen this situation occur in the real world.
thejoshinator
thejoshinator
Thanks Xai for your explanation of the arguments and self-assignment. I will apply them from now on :-) The reason I deemed it necessary to check for the validity of the operands is because the objects contain an array of HBITMAPS and I needed to varify that the source object had a valid HBITMAP from which to blit to the destination object. All I ended up having to do after hours of pouring over this was to check that the first element in the HBITMAP array contained a valid bitmap. This works because the object is such that the HBITMAPS are loaded from 0-n and deleted from n-0. However, I still think it necessary to check that *this is valid in the event of a dereferenced NULL pointer because it would mean (I think) that in every case, object_a = object_b would be safe, or at least non-lethal to the system, in case I ever forget to varify either argument before the assignment, and since the objects are visual, it's easy to tell whether or not the assignment happened. Consider the complete, modified code:
CTile& CTile::operator =(const CTile &src_tile){	// This function overloads the = operator so we can copy tiles correctly; *this is the left-hand operand(dest)	// and src_tile is the right-hand operand(src)	// If the src tile doesn't have any images, don't bother	if (!src_tile.images[0])		return *this; // return the pointer to the existing object	// Also, check to see if *this is valid        if (!this)                return *this; // return the pointer	int index = 0; // looping variable	// First we have to erase the deep stuff that's already there	for (index=0; index<MAX_TILE_IMAGES; index++)		this->images[index] = NULL;	// Copy the shallow members - no x,y because we want the tiles to keep their location and dimensions. We only 	// want to copy the image and image-related variables	this->anim_count = src_tile.anim_count;	this->anim_speed = src_tile.anim_speed;	this->curr_frame = src_tile.curr_frame;	this->next_frame = src_tile.next_frame;	this->num_frames = src_tile.num_frames;	this->width = src_tile.width;	this->height = src_tile.height;	// Now copy the deep members: the HBITMAPS	// First create 2 hdcs to use for blitting - one for the src tile and one for the dest tile	HDC dest_hdc = CreateCompatibleDC(*windraw.Get_Back_HDC()), // hdc for the dest tile		src_hdc = CreateCompatibleDC(*windraw.Get_Back_HDC());	 // hdc for the src tile	// Check the hdcs	if (!dest_hdc || !src_hdc)	{		DeleteDC(dest_hdc); // Delete the dest hdc		dest_hdc = NULL;	// Set it to NULL just in case				DeleteDC(src_hdc); // Delete the src hdc		src_hdc = NULL;	   // Set it to NULL just in case		return *this; // return the pointer	} // end if	// Create 2 old bmps to hold the old data	HBITMAP src_old_bmp = NULL,			dest_old_bmp = NULL;	// loop through all the images of each tile	for (index=0; index<MAX_TILE_IMAGES; index++)	{		// Create a compatible bitmap for the image		this->images[index] = CreateCompatibleBitmap(*windraw.Get_Back_HDC(), this->width, this->height);		// First select the frame into context and save the old one		src_old_bmp = (HBITMAP)SelectObject(src_hdc, src_tile.images[index]);		dest_old_bmp = (HBITMAP)SelectObject(dest_hdc, this->images[index]);				// Check the src_old_bmp		if (!src_old_bmp || !dest_old_bmp)		{				SelectObject(src_hdc, src_old_bmp); // Select the src old bmp back into context			DeleteObject(src_old_bmp); // Delete the src old bmp			src_old_bmp = NULL;		   // Set it to NULL			SelectObject(dest_hdc, dest_old_bmp); // Select the dest old bmp back into context			DeleteObject(dest_old_bmp); // Delete the dest old bmp			dest_old_bmp = NULL;		// Set it to NULL			this->images[index] = NULL; // Set the image to NULL			DeleteDC(dest_hdc); // Delete the dest hdc			dest_hdc = NULL;	// Set it to NULL just in case						DeleteDC(src_hdc); // Delete the src hdc			src_hdc = NULL;	   // Set it to NULL just in case 			return *this; // return the pointer		} // end if		// Blit the data from the src_hdc to the dest_hdc		BitBlt(dest_hdc, 0, 0, this->width, this->height, src_hdc, 0, 0, SRCCOPY);		// Select the old bmps back into context		SelectObject(src_hdc, src_old_bmp);		SelectObject(dest_hdc, dest_old_bmp);	} // end for	// Make sure the src_old_bmp still exists	if (src_old_bmp)	{		DeleteObject(src_old_bmp); // delete the bmp		src_old_bmp = NULL; // set it to NULL	} // end if	// Make sure the dest_old_bmp still exists	if (dest_old_bmp)	{		DeleteObject(dest_old_bmp); // delete the bmp		dest_old_bmp = NULL; // set it to NULL	} // end if	// Make sure the src_hdc still exists	if (src_hdc)	{		DeleteDC(src_hdc); // delete the hdc		src_hdc = NULL; // set it to NULL	} // end if	// Make sure the dest_hdc still exists	if (dest_hdc)	{		DeleteDC(dest_hdc); // delete the hdc		dest_hdc = NULL; // set it to NULL	} // end if	// return pointer to existing object	return *this;} // end CTile::operator=();

Should I keep this code, or does this still fall into the "it's the caller's fault" category and I should check every assignment of this particular object?
Desert Fox
Desert Fox
I never have my assignment operators check for validity of this or the argument. At most, I would just do an assert:
assert(this && &rhs);

I also highly recommend implementing the assignment operator in the form of copy construction.

IE:

CTile& CTile::operator = (const CTile& rhs){    CTile copy(rhs);    copy.Swap(*this);    return *this;}void CTile::Swap(CTile& other){   // Swap elements of *this and elements of other} 


A little wasteful? Maybe, but it's exception safe and it handles self-assignment gracefully
"Is life so dear, or peace so sweet, as to be purchased at the price of chains and slavery?" - Patrick Henry
ITGER
ITGER
You can have a null reference like so:

int *p = new int(5);
int &refP = *p;
delete p;

refP will then reference some garbage Be careful when you mix and match references with dynamically allocated memory!

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.

Sign in to reply to this topic.