Shivahn wrote:I have a class called SWImage that has a dynamic array of images in it, and a destructor that calls a function that checks to see if there's anything there
Just a side note... if you have something like
- Code: Select all
if (some_pointer != NULL)
delete some_pointer;
I suggest don't do that;
delete works fine if you pass it null. Simplifies and shortens the code.
So, it looked like this:
- Code: Select all
void HWImage::load(std::string filename, int index)
{
SWImage image(filename);
makeTexture(image, index);
}
Now, after a great amount of head bashing, I learned that I don't understand destructors. What appears to have been happening is that at the end of makeTexture, the compiler decides that image (which was passed in as an argument) is out of scope, and calls the destructor. makeTexture returns void, and then HWImage::load ends, calling the destructor on image (again).
So my first question is: Is that what was happening?
No.
What is much more likely is
makeTexture takes its parameter by value. Then there needs to be a copy made, which is done. When
makeTexture returns, that
copy is destructed.
However, unless you made the copy constructor work properly (and you don't describe doing that), what will happen is that the temporary will also have pointers to
image's data, and when the temporary is destructed it will delete that data, and then
image's destructor will again delete that data, giving you your double free.
SWImage should disable its copy constructor (make it private and don't provide a body, or, if you're using Boost, inherit
boost::noncopyable) or you need to correctly deal with what happens: you either need to do a deep copy of the data members or you need to reference count them (ideally with a smart pointer from Boost or TR1/C++11).
The same caution applies to
operator=, which your compiler supplies for you. Disable it in the same way or make it correct.
Now, I fixed the problem by making makeTexture take a SWImage pointer, and passing image in by reference (and changing all image.whatever() calls to image->whatever() ones). So my second question is: Is that the proper way to handle something like this? Or is there another, better way, or big problems with this one?
As long as you're happy with the behavior, it's fine. Objects are passed by pointer or reference all the time. You just have to be aware that modifications to
image made within
makeTexture will persist, of course.
There are a lot of private variables that makeTexture needs from SWImage that I want to keep private for good practice's sake. My first thought is just to create a bunch of public functions for SWImages, but that's ugly and seems suboptimal. In Scheme I'd do something like pass in a string and then do a case dispatch based on the string, returning whatever variable I need. First of all, is that how to do it in C++? Make a function like getProperty(std::string) with a switch structure? And secondly, if it is... what does it return? A void pointer that I handle by just being careful about what I'm asking for?
Separate getters for each field are the norm in C++. Don't do the
void* thing; that's the sort of thing that makes people like me whine in chat rooms with friends about terrible coding atrocities.
(In general, if what you're proposing includes the phrase "I'll just be careful about what I'm asking for" it's probably a bad idea. You only have to be wrong once and you wind up with something which can easily be a nightmare bug.)
