December 2017

Sun Mon Tue Wed Thu Fri Sat
          1 2
3 4 5 6 7 8 9
10 11 12 13 14 15 16
17 18 19 20 21 22 23
24 25 26 27 28 29 30
31            
My Photo

« VIDEO GAMES - Baldur's Gate: Dark Alliance II | Main | DIGITAL PHOTOGRAPHY - some PR Xmas '03 photos »

February 05, 2004

Comments

Paul

Just like every new() should have a delete(), every placement new should have a explicit destructor call. Unfortunately, I learned that The Hard Way.

Carlos

How very geeky. And in that vein, I present (from bbspot.com), the top 11 "Yo Momma Insults for Coders"

11. Yo momma's so crazy, she makes pi look rational.
10. Yo momma's so annoying, she made Clippy try to turn itself off.
9. Yo momma's so fat, she has to share resources with herself.
8. Yo momma's so insecure, she makes an unpatched copy of Microsoft IIS look like Fort Knox.
7. Yo momma's so stupid, she leaves possible buffer overruns in a "Hello World" application.
6. Yo momma's so boring, she makes debugging Prolog seem fun.
5. Yo momma's so fat, she uses FAT256.
4. Yo momma's code is so bloated, she makes assembly look like C.
3. Yo momma's so fat, she uses C++++.
2. Yo momma's so flea-ridden, she has more bugs than Tribes 2.
1. Yo momma's so ugly, she makes custom regex engines in perl look beautiful.

vince

Ugh. This code is an abomination.

Yes, I agree, it's clever and it works.

Here's the problem -- this struct has virtual functions. Therefore it is meant to be derived from -- yes, even if said virtuals came from a base class. Unless you are doing something to prevent derivation (say, make the constructors private), the following scenario is possible.

So what is going to happen when some poor innocent programmer (let's call him Vince) comes around, inherits a struct Foo from this struct, and calls ResetBlah()?

Bad stuff. What was once a Foo is now a Blah (in terms of its vtable). Plus any extra members in Foo weren't reset. For someone not aware of the internals of Blah, this bug could be maddening. "I don't get it! Suddenly my vtable is toast!"

Fine, you say. Just make ResetBlah a virtual and override it.

Well, now you've broken encapsulation and you've got Foo reaching down into details of Blah implicitly through a memset. So if the internals of Blah change such that something *can't* just be memset to zero, you have to change every class/struct that derives from it.

This is bad object design. Period.

Raist3d

Hey, thanks for pitching in.

You know, weird and funny but I can't help but chuckle a bit. It's not that what you say it's funny, but more the whole thing because of its original code.

Anyway, ho hom I can clearly see the issue of ResetBlah not being virtual and also I appreciate the reminder of enforcing the end of an inheritance hierarchy.

I'll check the code at work tomorrow....

Taking that out for a moment and just for mental fantasy purposes - if you call the destructor (virtual) on itself, should it matter if you call after that the memset to zero (i.e would you care if there's something up the hierarchy chain that "can't be set to zero?" since we destroyed our object?). Can you think of a concrete example?

The only thing that comes to my mind is if the structure was mapping to a set of hardware registers, and it just happens that writting zero from the memset would change or reset a state of that hardware.

Anyhow thanks for pitching in and giving a concrete example(s) of why this is a Bad Idea(TM).

- Raist

Grafik

All I've been wondering about this little bit of code is... Why would you even need to such nasty hacking with the object?

vince

class Blah
{
public:
Blah() { printf("Blah!\n"); }
virtual ~Blah()
{
printf("~Blah!\n");
}

virtual void foo()
{
printf("Blah::foo()\n");
}

void ResetBlah()
{
this->~Blah();
memset(this, 0, sizeof(*this));
new (this) Blah;
}

int mPhil;
};

class Foo : public Blah
{
public:
Foo() { pSomething = new int; printf("Foo!\n"); }
virtual ~Foo() { delete pSomething; printf("~Foo!\n"); }

virtual void foo()
{
printf("Foo::foo()\n");
}
private:
int *pSomething;
};

int main(int argc, char *argv)
{
Foo f;
Blah *pFoo =

pFoo->foo();
pFoo->ResetBlah();
pFoo->foo();

return TRUE;
}

When you run the above program, this is the output:

Blah!
Foo!

Foo::foo()

~Foo!
~Blah!

Blah!

Blah::foo()


In addition, Foo::pSomething has been leaked and I get a heap corruption error on exit.

get it?

vince

BTW, the heap corruption occurs because it attempts to call Foo::~Foo() again (since its a frame object) when the stack unrolls as it exits main, so it's doing a double free.

Raist3d

@Grafik
Someone at work wanted to set a whole thing to zeroes, fast, and thought this was a clever "kewl" way to do it. For reference, this is a "C" company that is now bumping into C++. I personally wouldn't have done this because it wouldn't enter my realm of possibilities and when I saw it amongst all the laughing for the cleverness I untuitively felt there was something wrong with it (hence the title of this thread).

@Vince
Oh Vince, maybe I didn't make myself clear, but I did get what you illustrated in your example the first time (though I didn't consider the double-delete scenario). When I mentioned the "mental fantasy" was on the assumption of implementing a virtual ResetBlah().

I was wondering on a concrete situation that would make Blah not resetable to zero - purely out of genuine curiosity- not out of "defending" this way of doing things.

Thanks again for your commments.

Raist3d

More comments after talking to the guy involved:

- Blah is not designed to be allocated on the stack
- ResetBlah() is a virtual already
- The most "sub" class in this hierarchy has to implement ResetBlah. His answer here was that it is an issue that if you have virtuals for clasess in general, often for them to make sense (some or all) you need to implement in the subtypes. How to enforce that the most "outmost" class needs to implement this?

- After the destructor call, nobody should care if you do the memset zero including the base class memory because after the dtor call, this memory should not be something anybody should care about.

Another context datum: there's no use of the C++ heap.

Anyway, I learned a couple of things from this back and forth.


Raist3d

Just one more thing:

The co-worker doing this has actually used C++ for tools before, so he's not "in the C crowd." This is irrelevant of whether what he is doing rihgt now is right or wrong, but don't want to give the impression nobody here, particularly him, know about C++.

- Raist

The comments to this entry are closed.