Jump to content
  • Advertisement
Sign in to follow this  
CyberSlag5k

C++ - Delete, copy constructor, and assignment operator

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

I have a question on proper coding practice. I have written two classes, texture and bitmap. A texture has a bitmap member (a pointer named m_Bitmap). Because a Bitmap has resources that must be managed (dynamically allocated memory), if I create a Texture which creates a Bitmap, and then later copy some other Texture into my original Texture, I have to clean up the Bitmap I created before. So I've written a Copy method that both the copy constructor and the assignment operator will call. The question is, should I explicitly clean up the Bitmap in Texture::Copy:
void Texture::Copy(const Texture& newTexture)
{
   if(m_Bitmap != NULL)
   {
      delete m_Bitmap;
      m_Bitmap = NULL;
   }
   m_Bitmap = new Bitmap(newTexture.m_Bitmap);
}


Should I do it in Bitmap::Copy:
Bitmap::~Bitmap()
{
   Delete();
}

void Bitmap::Delete()
{
   if(m_Data == NULL)
      return;

   for(int i = 0; i < m_Width; i++)
      delete [] m_Data;
   delete m_Data;
   m_data = NULL;
}

void Bitmap::Copy(const Bitmap& newBitmap)
{
   Delete();
}

void Texture::Copy(const Texture& newTexture)
{
   m_Bitmap = new Bitmap(newTexture.m_Bitmap);
}


Or should I do both:
Bitmap::~Bitmap()
{
   Delete();
}

void Bitmap::Delete()
{
   if(m_Data == NULL)
      return;

   for(int i = 0; i < m_Width; i++)
      delete [] m_Data;
   delete m_Data;
   m_Data = NULL;
}

void Bitmap::Copy(const Bitmap& newBitmap)
{
   Delete();
}

void Texture::Copy(const Texture& newTexture)
{
   if(m_Bitmap == NULL)
   {
      delete m_Bitmap;
      m_Bitmap = NULL;
   }
   m_Bitmap = new Bitmap(newTexture.m_Bitmap);
}


Now I believe all of those will get the job done (I just typed them here and now so there may be a typo or something I didn't think of), and the last solution is particularly redundant. However I actually think it might be the best solution, as it combines the safety of the Bitmap's copy cleaning up the resources (so if anyone forgets to do so when making such a copy it still gets done), as well as clearly expresses its intent to clean up the bitmap's resources before re-assigning the pointer. So which, if any, of these solutions is the proper way to go about handling a situation like this? Also, is this: delete m_Bitmap; m_Bitmap = NULL; redundant? Does delete set m_Bitmap to NULL automatically? I've always just done it myself.

Share this post


Link to post
Share on other sites
Advertisement
Personally, I would replace your m_Bitmap pointer with a std::auto_ptr<Bitmap>. Then, in the copy constructor for Texture, call m_Bitmap.reset(new Bitmap(copyObject.m_Bitmap));

This will set the auto_ptr to point to a new Bitmap object which is copy-constructed from the other Texture's m_Bitmap. The old Bitmap will be freed automatically. Better yet, this removes the need for Texture to manually free m_Bitmap in the destructor.


(Oh, and delete does not automatically set the pointer to NULL, so doing it yourself is a very good idea. delete NULL; is defined to have no effect, so no worries about deleting already-NULL pointers, either.)

Share this post


Link to post
Share on other sites
Quote:
Original post by ApochPiQ
(Oh, and delete does not automatically set the pointer to NULL, so doing it yourself is a very good idea. delete NULL; is defined to have no effect, so no worries about deleting already-NULL pointers, either.)
Well, in most implementations it does not set the pointer to NULL, although the standard leaves it to the implementor. Stroustrup himself hoped that most implementors would do so, but they mostly did not. You certainly can't assume it anyway.

Share this post


Link to post
Share on other sites
Yes, the auto pointer suggestion is a good one. Thank you.

However the question remains, as there are other resources besides dynamically allocated memory to be cleaned up. The texture itself, for example, will require a call to glDeleteTextures before I can assign it a new texture ID. So where in the grand copy scheme do you guys think that should take place?

Thanks again, ApochPiQ.

Share this post


Link to post
Share on other sites
Wrap your resource in an object, create the resource in the constructor and destroy it in the destructor. This way, the lifetime of the resource becomes is the same as the lifetime of the object, which you can control with normal or smart pointers depending on your needs.

Alternatively, some smart pointers, such as boost::shared_ptr, allow you to specify the deletion function (which may be something other than delete).

Share this post


Link to post
Share on other sites
I'm with ToohrVyk on this - your design suggests that you are not using RAII properly. Each instance of Texture should correspond to precisely one texture resource. If you want a different texture resource, get a different instance of Texture.

In general, if you find yourself in a situation where you need to re-seat an object with instance data from another object, there's a good chance you have a design flaw.

Share this post


Link to post
Share on other sites
Maybe I'm missing something, but I thought that was exactly what I was trying to do...

Say I had 3 textures and 2 bitmaps:

Bitmap grassBMP("grass.bmp");
Bitmap rockBMP("rock.bmp");

Texture texA(grassBMP);
Texture texB(grassBMP);
Texture texC(rockBMP);

Even though texA and texB look exactly the same and contain the exact same image VALUES, they reference two completely different blocks of memory because I do deep copies of the bitmaps. Each also has a completely different texture ID.

When I do this:

texB = texC

texB's old bitmap is deleted, as is the OpenGL texture associated with it. It then does another deep copy of texC's bitmap, is assigned a new texture ID (this step may not be necessary, I'm not sure), and a new texture is generated from the newly created (albeit copied) bitmap.

So each object references a completely separate set of data and a completely different texture (as registered with the OpenGL "server") as the others. Because of this, when a copy happens the old objects must be completely cleaned up before the new assignment happens, but because each references entirely separate data/textures, there are no conflicts with the other objects.

Is this not correct?

Share this post


Link to post
Share on other sites
Too much work. Why not simply do the following:

class Texture 
{
GLTexture glTexture;
Bitmap bmp;
public:
Texture & operator=(const Texture& other)
{
bmp = other.bmp;
glTexture = GLTexture(bmp);
}
};


Share this post


Link to post
Share on other sites
So are the old values for bmp and glTexture getting cleaned up in their respective assignment operators, then? This is precisely what I was asking with the original question.

Share this post


Link to post
Share on other sites
Right. So, if you have a GLTexture class as well, I had just misunderstood what you were saying about the GL texture handling.

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!