Sign in to follow this  
Sean_Seanston

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

Recommended Posts

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?

Share this post


Link to post
Share on other sites

There is something else going on.  C++ programs use *this all the time.

Vector2d& Vector2d::operator= (const Vector2d& param)
{
  x=param.x;
  y=param.y;
  return *this;
}

There should be no problem pass a (this*) to a function.  

 

 


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?

 

As far as this part goes, it can be confusing.  C++ function arguments are always "pass-by-value".  Everything that goes into a function is copied.  That's why you pass a pointer or reference instead of an object.  It can be a lot cheaper to copy a 32bit pointer than a huge object.  For instance, when you pass a pointer and then access the thing it points to, you are using a copy of the pointer that points to the same object.

 

As an example, what does this do?

void func( int* intPointer ) {
   intPointer = new int{2};
}

void test() {
   int x = 4;
   int* intPointer = &x;
   func( intPointer );
   std::count << *intPointer << std::endl;
}

If the int* was passed by reference, then we have changed it to point to a different value.  Is that what happens?

Edited by Glass_Knife

Share this post


Link to post
Share on other sites

There's nothing wrong with dereferencing the this pointer, your error has most likely something to do with your code(I'm guessing the second parameter is of type ClassA&).

 

 


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 depends on the argument type in functionB, it could be by value, reference, const reference etc.

Edited by Mussi

Share this post


Link to post
Share on other sites


Just as a point but a pointer is not necessarily 32 bits and you should never ever write code that actually relies on the exact size of a pointer. This is what causes a lot of bugs in software when switching from 32-bit to 64-bit.

 

Yes, you're right.  Good point.

Share this post


Link to post
Share on other sites

There should be no problem pass a (this*) to a function.

 

Interesting... must be something strange going on then. I'll elaborate and describe the situation properly later. I was just hoping it would be something like that so I wouldn't have to track down this apparent weirdness unsure.png .

 

As far as this part goes, it can be confusing.  C++ function arguments are always "pass-by-value".  Everything that goes into a function is copied.  That's why you pass a pointer or reference instead of an object.  It can be a lot cheaper to copy a 32bit pointer than a huge object.  For instance, when you pass a pointer and then access the thing it points to, you are using a copy of the pointer that points to the same object.

 

Hmm... so you'd have to pass a pointer by reference I assume in order to change what it points to from within a function?

 

What is the signature of functionB()? If functionB does takes ClassA by value, is the class safely copyable, i.e. obeys the rule of three?

 

Let's see... apart from 2 static members (which I assume are irrelevant) it's composed of ints, a bool, 2 custom structs that are just ints/bools and... aha! I think we might be onto something here... the last 3 data members are all instances of my custom BitmapFont class, which is actually what the problem is centred around in the first place.

 

What I find strange though is that the problem is occurring JUST BY PASSING (*this) into the function, even when that function is empty or at least not doing anything in any way with the argument corresponding to (*this).

I suppose that probably just makes it easier to debug tho, narrowing things down...

 

b) *pointer where `pointer` is an actual pointer will dereference the pointer. The result is always an lvalue reference. `this` is always an actual pointer. Hence *this is always a reference, not an object. Simple.

 

This must have something to do with it then...

 

Though if anything, I would have thought that being a reference would make the chance of e.g. things being copied wrong, less likely.

 

Anyway, proper description of the situation coming up... just hope I can be concise...

Share this post


Link to post
Share on other sites

I've been building a system for keeping track of a game's date and time based upon the real-life Gregorian calendar. So you might have a strategy game where you want to display the date and time as "04/02/2043 12:43", and as time passes the system will accurately keep track of the number of days in a month including leap years etc.

To do this I have 3 main components:

A struct called Time which keeps track of seconds, minutes and hours.

A struct called Date which does the same for days, months and years.

A class called GameDateTime which has Time and Date structs and is used to coordinate the whole thing, e.g. when the time is 23:59:59 and advances one second, the time will automatically reset to 00:00, but GameDateTime ensures that this causes its Date object to also advance by 1 day.

 

GameDateTime also has 3 objects of the class BitmapFont, which are used to display the time, date and ticks since the epoch time of the gameworld. Maybe it isn't ideal for them to be here in the first place, or maybe it's alright, either way I mostly just have them there for convenience while I'm working on implementing the whole system and checking that it works.

 

The problem is:

- I have a function in GameDateTime called setDateTime. It takes a Date and a Time struct, the idea being to be able to change a GameDateTime's date and time values to any arbitrary value (and still keep track of time since the epoch etc.).

- In order to get the epoch counter right, I need to know the time between the new date/time and the epoch time/date. To do this I made a static function in GameDateTime called getMinsBetweenGameDateTimes, which takes 2 GameDateTime objects and works out the time between them in minutes.

- As far as I can see, everything is working as it's supposed to be in terms of mathematics etc., and getMinsBetweenGameDateTimes seems not to cause any problems when given 2 GameDateTime objects that don't involve (*this), like I've said.

- However, when a GameDateTime object's setDateTime() is called (I know because I was testing by using a "Y" keypress to make the date jump forward by a year) and it in turn calls getMinsBetweenGameDateTimes( epochDateTime, (*this) ), the text of all 3 BitmapFont objects instantly disappear and don't return.

 

Make sense to anyone?

 

I find it strange because none of the functions that get called appear to have anything directly to do with the BitmapFont objects (they receive their values in a separate draw() function) and I can't understand that they'd be somehow getting overwritten with improperly set up objects. I would understand maybe if passing (*this) was somehow, through improper copying, corrupting the values of the BitmapFont objects that the function would then be accessing, but the function only ever deals with the Time and Date structs of the GameDateTime objects.

 

Could passing (*this) somehow be causing the object's data members to actually change in some way? Well, any of my theories are probably going to be way off...

 

EDIT: I see it also happens if I call something as simple as

GameDateTime gdt = (*this);

Edited by Sean_Seanston

Share this post


Link to post
Share on other sites
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.

Share this post


Link to post
Share on other sites

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? mellow.png

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

The solution is to understand the [url="http://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29"]Rule of Three[/url] (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.

Share this post


Link to post
Share on other sites

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.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this