Sign in to follow this  
CyberSlag5k

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

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[i];
   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[i];
   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
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
Excellent. I have the answer I need now, have the objects handle their clean-up behind the scenes in the copy constructor and assignment operators. If anyone would care to comment further on the topic, I'd be interested to hear any other opinions on the subject (perhaps in favor of the expressly cleaning up the resources externally anyway for readability's sake), but for now I shall proceed with this method, as I feel it is indeed sufficient (and can actually be less confusing).

Thank you, ApochPiQ and ToohrVyk.

Share this post


Link to post
Share on other sites
Quote:
Original post by iMalc
Stroustrup himself hoped that most implementors would do so, but they mostly did not.


Which is strange, because it goes against the language philosophy to do so :)

Anyway. Objects are supposed to clean themselves up.

Does the Texture *need* to hold its Bitmap *by pointer*? Why? If textures can *share* a Bitmap, you are in for a world of pain if you try to do it yourself; use Boost::shared_ptr. If the Bitmap class is polymorphic - well, I'm not sure what to tell you if you're making a *Bitmap* class polymorphic :)

Probably, the Texture can hold a Bitmap by value. If we make the Bitmap properly clean up after itself, then the Texture can just copy and assign "normally" (i.e. without having to write anything for those operators), because the default will copy or assign member-wise (i.e. calling those operators recursively for each member, including bases), not bit-wise.

Anyway, a Bitmap obviously *needs* a dynamic allocation ;) because it can vary in size. Also, it will "own" this allocation, so we can use this general sort of approach, but:

Don't implement a separate Copy() function, for normal situations. Instead, implement a swap() function, and implement the assignment operator using a copy and a swap. The idea is that at the end of the operator=, the destructor kicks in for the copy, which will automatically clean up the stuff that you swapped into it, while the "current" object now holds the stuff that was in the copy (which is a clone of the data from the RHS of the assignment).

That way, if the copy throws an exception (e.g. a std::bad_alloc), nothing Bad(TM) will happen in the assignment operator (except for the operation failing, of course) - and as long as the copy constructor is exception-safe (hint: don't directly dynamically allocate more than once, or perform a possibly-throwing operation after allocating something, in any initializer list; and if you can throw in the constructor body, make sure any direct, local dynamic allocation is cleaned up), the assignment operator will be too (since the swap can't throw an exception). The swap function is often made public because it often turns out useful :)

When you *do* make something polymorphic, it's common to implement a virtual member function called clone(), which *also* uses the copy constructor (returning a pointer to a dynamic copy of self). This is done because just calling a copy constructor doesn't work polymorphically - if you call it on a reference-to-Base (or invoke through a pointer-to-Base), the copied thing is a Base instance regardless of the run-time type of the referred-to instance.

Also, don't factor code out of constructors or the destructor until a need is established.


class Texture {
Bitmap m_Bitmap; // not a pointer!
};

class Bitmap {
// Lots of stuff left out, of course.
int w, h;
char* m_Data; // or whatever type

int size() { return w * h; }

public:
void swap(const Bitmap& other) {
std::swap(m_Data, other.m_Data);
std::swap(w, other.w);
std::swap(h, other.h);
}

Bitmap(const Bitmap& other) : w(other.w), h(other.h), m_Data(new char[size()]) {
std::copy(other.m_Data, other.m_Data + size(), m_Data);
}

Bitmap& operator=(const Bitmap& rhs) {
swap(Bitmap(rhs));
return *this;
}

~Bitmap() {
delete[] m_Data;
}
};


To answer your question about nulling things out: it's not redundant in the sense that it won't be guaranteed to be done, but it *is* a sign of bad design. When you use RAII in the illustrated way, you won't need to null out after deletion because you only delete things that are "about to die". It doesn't matter that m_Data is dangling at the end of Bitmap::~Bitmap(), because that particular m_Data value is provably no longer accessible to the rest of the program (unless someone invoked undefined behaviour or did something else blatantly unsafe) - simply by static analysis of the Bitmap class (i.e. m_Data is private, and no two instances ever share a pointer value, because the assignments only come from new allocations).

Share this post


Link to post
Share on other sites

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