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).