Back to General and Gameplay Programming

Is it a very bad idea for a class to pass (*this) into a function? Strange bug...

Sean_Seanston · 2015-01-29T18:08:28

Just wondering if it's a known source of errors for a class to do something like: void ClassA::functionA() { variableX = functionB( classA1, (*this) ); } ??? Because I was experiencing a very strange error last night when trying something like that, and it turned out the problem went away if I stopped passed (*this) and instead e.g. created another object of type ClassA that copied the values of (*this) and passed that in instead. I admit it does seem like a strange, probably sloppy, construction, but I've never heard of it being something to avoid and if it was so specifically bad I would've expected some form of compiler error/warning either when compiling or running/debugging. The problem definitely seems to come from solely passing (*this), even when the function does nothing. The bug I get then is quite puzzling to me, because it seems to be affecting things that are almost unrelated... the only thing I can figure is that an object passing itself like this might be somehow able to cause some kind of screw up/corruption with that object's data members in some circumstances. At first I thought maybe some kind of infinite loop but the program is still running. So is this known to cause unexpected behaviour and to be avoided like the plague, or should I get into explaining my code quite specifically? Also, I know that "this" is a pointer... and since * is used to dereference a pointer, does that mean passing (*this) is pass-by-value? That was my assumption at least, and hence I can't figure out why it would be a problem and passing in an object with the same values isn't. Unless it's some kind of pass-by-reference?

General and Gameplay Programming Programming

Started by Sean_Seanston January 25, 2015 02:18 PM

14 comments, last by Sean_Seanston 9 years, 2 months ago

rip-off

11,000

January 28, 2015 08:08 PM

What is likely happening is that the BitmapFont is not copyable. That is, when you create a copy, and the copy goes out of scope, it destroys resources that the "original" is also using.

Something like this:


$ cat test.cpp
#include <iostream>
class Bad {
public:
    Bad() : pointer(new int(42)) {
        std::cout << "constructor: " << this << ", pointer: " << pointer << '\n';
    }

    ~Bad() {
        std::cout << "destructor: " << this << ", pointer: " << pointer << '\n';
        delete pointer;
    }

    int *pointer;
};

int main() {
    Bad original;
    Bad copy = original;
}
$ g++ test.cpp && ./a.out
constructor: 0x7fffeb5038a0, pointer: 0x131d010
destructor: 0x7fffeb5038b0, pointer: 0x131d010
destructor: 0x7fffeb5038a0, pointer: 0x131d010
*** Error in `./a.out': double free or corruption (fasttop): 0x000000000131d010 ***
Aborted (core dumped)

As you can see, there are two "this" pointer values, but only one "pointer" value. The destructor causes the pointer in "original" to also be invalidated.

blog | twitter

Sean_Seanston

880

Author

January 28, 2015 11:30 PM

What is likely happening is that the BitmapFont is not copyable. That is, when you create a copy, and the copy goes out of scope, it destroys resources that the "original" is also using.

Ahhh... of course, that makes sense. I should have noticed that possibility myself.

Now just checking the destructor... if I comment out the destructor of the underlying SpriteSheet object then it does stop happening. Yay.

Thanks for tracking that one down. Left me quite confused for some time.

Yes... I see what it ultimately leads to now; the GLuint variables referring to the texture and sampler get deleted when the texture wrapper class goes out of scope, and the VBOs of SpriteSheet do likewise.

As for a solution... I've been trying to think this through... and doing the ordinary deep copy of a pointer seems like it wouldn't be very straightforward. Am I right in thinking that to give a copy its own copy of the texture (and thereby allow it to release that texture on destruction), you'd have to go through loading the whole texture again, or is there another way in OpenGL?

Would it be better to work around it instead, and remember never to copy these classes like that? i.e. Just keep using my method of creating a new GameDateTime object with the same values as the current one. Sounds like bad practice to leave such a potential bug lying around, but the other way seems kind of elaborate and maybe wasteful to me.

The only other option I can foresee is giving it some way of knowing if a texture is in use, and only selectively deleting it if nothing else is using it. Not sure how practical that would be. Maybe it would work by incrementing some counter when the copy constructor is called, and then decrementing it on destruction? Feels kind of sloppy to me though and like it might be covering up bad design.

Or am I just tired and the solution is much simpler and more obvious than I realize?

Washu

7,836

January 29, 2015 02:15 AM

Sounds like this object should not own the texture at all, that should be owned by someone else instead. Someone who knows the actual lifetime of the texture.

In time the project grows, the ignorance of its devs it shows, with many a convoluted function, it plunges into deep compunction, the price of failure is high, Washu's mirth is nigh.

rip-off

11,000

January 29, 2015 11:38 AM

The solution is to understand the Rule of Three (this rule has recently grown a bunch of different names, but I think this still is the best name).

So, your BitmapFont class or potentially some of the other classes it uses, should be marked as "noncopyable". You can achieve that in C++11 by "deleting" the copy constructor and assignment operator, and in pre C++11 you can simulate this by declaring them private and not implementing them.

This will address the issue you mentioned of the silent bug lying around. Anywhere you are copying these objects should now give a compilation error (or possibly a link error).

The next thing to consider is the design. As Washu says, this design seems weak.

blog | twitter

Mussi

4,453

January 29, 2015 05:46 PM

The solution is to understand the Rule of Three (this rule has recently grown a bunch of different names, but I think this still is the best name).

Rule of 0/3/5

Sean_Seanston

880

Author

January 29, 2015 06:08 PM

Sounds like this object should not own the texture at all, that should be owned by someone else instead. Someone who knows the actual lifetime of the texture.

Hmmm... yes, that sounds about right.

So, your BitmapFont class or potentially some of the other classes it uses, should be marked as "noncopyable". You can achieve that in C++11 by "deleting" the copy constructor and assignment operator, and in pre C++11 you can simulate this by declaring them private and not implementing them.

This will address the issue you mentioned of the silent bug lying around. Anywhere you are copying these objects should now give a compilation error (or possibly a link error).

The next thing to consider is the design. As Washu says, this design seems weak.

Cool, sounds interesting.

I'll have a look and see what I can improve... thanks.

Is it a very bad idea for a class to pass (*this) into a function? Strange bug...

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Is it a very bad idea for a class to pass (*this) into a function? Strange bug...

This topic is closed to new replies.

Popular Topics

Recommended Tutorials

Reticulating splines