The secure destructor
-
The company I work for contracts us to a customer which is a large government-controlled entity, hereafter GCE. Our task there is to maintain the zillion of in-house applications they use, aka The System.
Applications from The System generally suffer from a number of ailments, from not-invented-here syndrome to just-plain-crap syndrome and not-understanding-wtf-we-are-doing syndrome, aka cargo-cult programming. An example of the former would be that using bools is deprecated. Instead, there are Booleans. And what is a Boolean? A typedef for a short, of course. In which way is it better than a bool? Why, it was developed at GCE, silly! We have it in a source file in our code base, along with two homemade consts YES and NO instead of these foreign true and false! Sadly, this is probably one of the sanest and most harmless examples we have around -- no, there is no FILE_NOT_FOUND, and YES does not equal 0 either.
An example of the latter could be the following paradigm. As we all know, duplicating code is bad. And classes often have several constructors that perform mostly similar tasks, except for the bits where the parameters are involved. So, in just about every class in The System, the constructor only copies its parameters in the newly created object, then calls void Classname::constructor(void) which performs the actual work. Still not so bad, you say? Well, here's the kicker: destructors are subjected to the same rules.
However, this pales in comparison with what I've found the other day. I was wading through the code of the mother class of a MDI application, and found this:
void AppSomething::destructor(void)
{
if (m_pChild1 != NULL) {
if (m_pChild1->isDestroyed() == NO) {
delete m_pChild1;
}
}
if (m_pChild2 != NULL) {
if (m_pChild2->isDestroyed() == NO) {
delete m_pChild2;
}
}
// snip the same code repeated for every child frame
// and then other stuff
}
Dear God, I thought, please let it not be as stupid as it looks. Alas, God had forsaken me this day.
Boolean GceChild1::isDestroyed(void)
{
return m_bDestroyed;
}
Aside from the member definition in GceChild1.h (private, so God in fact had had a little mercy on my soul), a search returned just two other mentions of m_bDestroyed:
void GceChild1::constructor(void)
{
// snip real work
m_bDestroyed = NO;
}
void GceChild1::destructor(void)
{
// snip real work
m_bDestroyed = YES;
}
(Of course, the same monster was copypasted in every GceChild class from 2 to I-don't-remember-how-many.)
I guess whoever wrote that wanted to really, really make sure the application wouldn't go and delete stuff it no longer owned.
-
Ugh, looks like somebody was too lazy/stupid to do proper deep copying. Actually, I'm definitely going with stupid.
-
What is it that makes so many people think you can access the data in a class after it has been destroyed? I've come across that little issue distressingly frequently.
-
@thosrtanner said:
Because windows allows it 99% of the time.What is it that makes so many people think you can access the data in a class after it has been destroyed? I've come across that little issue distressingly frequently.
-
I think this is a cargo-cult implementation of reference count. The "isDestroyed" field might have been meant to prevent actual deletion. Or they might have a circular data structure, where they try to make sure that an instance is not deleted twice. However, in that case, the name is rather misleading and it should be documented in great big letters, because if you change the order of deletion and setting the variable (or decreasing the ref count), disasters are going to happen at the worst possible moment...
No, I don't speak from experience. Why do you ask?
-
@Daid said:
Because windows allows it 99% of the time.
What exactly does Windows have to do with it? I'm dying to hear this.
-
be fair. It works most of the time on 99% of all systems. However, it is wrong 100% of the time. And the people who do it ALWAYS look at you as if you're an idiot when you explain why it might not be a terribly good idea.
-
@Daid said:
Because windows allows it 99% of the time.
Are you really this stupid? Or just a moronic /. script kiddie who accidentally ended up here? I'm betting on both.
Idiotic anti-MS/Windows bashing for absolutely no reason doesn't impress anybody with anything but the assurance that you're an imbecile. If that was your goal, you succeeded.
-
This sounds almost as bad as Symbian OS's way of using objects...
