• Announcements

    • khawk

      Download the Game Design and Indie Game Marketing Freebook   07/19/17

      GameDev.net and CRC Press have teamed up to bring a free ebook of content curated from top titles published by CRC Press. The freebook, Practices of Game Design & Indie Game Marketing, includes chapters from The Art of Game Design: A Book of Lenses, A Practical Guide to Indie Game Marketing, and An Architectural Approach to Level Design. The GameDev.net FreeBook is relevant to game designers, developers, and those interested in learning more about the challenges in game development. We know game development can be a tough discipline and business, so we picked several chapters from CRC Press titles that we thought would be of interest to you, the GameDev.net audience, in your journey to design, develop, and market your next game. The free ebook is available through CRC Press by clicking here. The Curated Books The Art of Game Design: A Book of Lenses, Second Edition, by Jesse Schell Presents 100+ sets of questions, or different lenses, for viewing a game’s design, encompassing diverse fields such as psychology, architecture, music, film, software engineering, theme park design, mathematics, anthropology, and more. Written by one of the world's top game designers, this book describes the deepest and most fundamental principles of game design, demonstrating how tactics used in board, card, and athletic games also work in video games. It provides practical instruction on creating world-class games that will be played again and again. View it here. A Practical Guide to Indie Game Marketing, by Joel Dreskin Marketing is an essential but too frequently overlooked or minimized component of the release plan for indie games. A Practical Guide to Indie Game Marketing provides you with the tools needed to build visibility and sell your indie games. With special focus on those developers with small budgets and limited staff and resources, this book is packed with tangible recommendations and techniques that you can put to use immediately. As a seasoned professional of the indie game arena, author Joel Dreskin gives you insight into practical, real-world experiences of marketing numerous successful games and also provides stories of the failures. View it here. An Architectural Approach to Level Design This is one of the first books to integrate architectural and spatial design theory with the field of level design. The book presents architectural techniques and theories for level designers to use in their own work. It connects architecture and level design in different ways that address the practical elements of how designers construct space and the experiential elements of how and why humans interact with this space. Throughout the text, readers learn skills for spatial layout, evoking emotion through gamespaces, and creating better levels through architectural theory. View it here. Learn more and download the ebook by clicking here. Did you know? GameDev.net and CRC Press also recently teamed up to bring GDNet+ Members up to a 20% discount on all CRC Press books. Learn more about this and other benefits here.
Sign in to follow this  
Followers 0
blueshogun96

Class instances gone wrong!

26 posts in this topic

Dear C++ newbies, experts and all of you people in between,

 

Today, I ended up doing something really stupid, and I don't want any of you doing what I did today.  If breakpoints and debugging features are part of your IDE, please, for the love of God, please use them!  sad.png

 

While working on a new game concept I thought of a couple of days ago, I stopped and thought to myself "I'm going to use C++ this time and not use pure C to avoid the mistake of letting my cross platform code base getting messy again now that I'm more comfortable with the various mobile and desktop APIs and what not".  So I start writing my basic application classes with portability in mind.  For reasons I don't feel like getting into, I made my decision to use OpenGL via GLUT on MacOSX.  I know, I know, I've never recommended GLUT in the past, but this time, I felt like using it since the game only uses either a mouse or a touch screen anyway (except for the old fashioned exit procedure; the escape key).

 

I don't know about you, but I have a tendancy to think about how I want my app to execute!  I liking having my main() function contain as little code as possible.  With that in mind, I ended up writing my main.cpp file to look like this:

 

game_app_t* app = NULL;

void exit_func()
{
    if( app )
        delete app;
}

//-----------------------------------------------------------------------------
// Name: main
// Desc: Program entry point.
//-----------------------------------------------------------------------------
int main( int argc, char* argv[] )
{
    atexit( exit_func );
    app = new game_app_t;

    return 0;
}

 

Okay, hold on, I promise you the problem with this code is NOT that obvious!  You're probably thinking "Yeah, no s@#% sherlock! You didn't call delete or your game loop function!"  Actually, I did.  The class constructor calls the initialization function where glutMainLoop is called upon success.  Instinctively, I check my code to verify that everything is uninitialized properly, even more than I check for successful initialization.  So I put a breakpoint inside of exit_func to verify that it's being called (well, I knew the function works, but I habitually check anyway).  Just in case you're wondering, the code for my class (the initialization stuff) looked like this:

 


#include "game.h"
#include <assert.h>


/* Useful macros */
#define safe_delete(x) if(x) { delete x; x = NULL; }
#define fail_safe(x) if(!x) return;

/* Global class instance */
game_app_t* This = NULL;


/* Default constructor/deconstructor */
game_app_t::game_app_t()
{
    this->init();
}

game_app_t::~game_app_t()
{
    this->uninit();
}

/* Initialization and uninitialization */
bool game_app_t::init()
{
    /* Set global class instance */
    This = this;
    
    /* Initialize OpenGL */
    m_ogldrv = new ogldrv_t();
    assert( m_ogldrv != NULL );
    
    if( !m_ogldrv->init( 0, NULL, 640, 480, GLUT_RGBA | GLUT_DOUBLE | GLUT_DEPTH, "Loop-til" ) )
        return false;
    
    /* Set default callbacks */
    glutDisplayFunc( this->display_func );
    glutIdleFunc( this->idle_func );
    glutKeyboardFunc( this->keyboard_func );
    glutReshapeFunc( this->reshape_func );
    glutMouseFunc( this->mouse_click_func );
    glutPassiveMotionFunc( this->mouse_move_func );
    
    /* Start the main loop */
    glutMainLoop();
    
    return true;
}

void game_app_t::uninit()
{
    safe_delete( m_ogldrv );
}

/* Event callback functions */
void game_app_t::display_func()
{
    fail_safe(This->m_ogldrv);
    
    /* Clear the rendering buffers */
    This->m_ogldrv->clear_buffers();
    
    /* Swap the double buffer */
    This->m_ogldrv->swap_buffers();
}

void game_app_t::idle_func()
{
    glutPostRedisplay();
}

/* Blah blah blah */

 

 

And yes, the event callback functions were declared as static, so I needed an extra copy of my this pointer.  Since there only needs to be one instance of this class, it doesn't really matter, IMHO.

 

This is where I found my REAL problem: app is still NULL!  I was kinda dumbfounded when I realized that the class instance didn't get uninitialized (hence my deconstructor was never called).  I spent a few minutes going over the code to see why this wasn't working.  The last thing I wanted was to be leaking memory every launch.  After a few more moments of scratching my head, I realized that the problem was that it the call to "new" doesn't return anyway.  So the logical thing to do was to use the copy of my this pointer and delete that explicitly instead.  After that thought, I added another function:

 

void game_app_t::exit_func()
{
    safe_delete(This);
}


/* Added after setting all the other callbacks */
atexit( this->exit_func );

 

So, I added a breakpoint, ran my app in debug mode, and this time, my class instance not only gets deleted properly, but now my deconstructor gets called, and calls the uninit function.  Bam, problem solved!  Now my main function looks like this:

int main( int argc, char* argv[] )
{
    new game_app_t;

    return 0;
} 

 

So short, so sweet.  Isn't C++ great once you use it properly?

 

So, the moral of the story is- "Don't be as stupid as Shogun?"  Yeah, that too, but please, use your breakpoints and always, ALWAYS, cover your digital butt!  I've seen a handful of rookie game programmers who have never learned to use breakpoints (I was one of those).  What I did was dumb.  I made this mistake for you, please don't do it for yourself. ph34r.png

 

Shogun.

 

 

2

Share this post


Link to post
Share on other sites

I have a question. Why are these macros and not methods or even functions?

/* Useful macros */
#define safe_delete(x) if(x) { delete x; x = NULL; }
#define fail_safe(x) if(!x) return;
 
0

Share this post


Link to post
Share on other sites

Another question. Why are you using

/* Global class instance */
game_app_t* This = NULL;
 

 

and not

 

int main( int argc, char* argv[] )
{
    app = new game_app_t();
    /* where game_app_t is an actual class */
    return 0;
}
 
0

Share this post


Link to post
Share on other sites

Why 'new' tongue.png

int main()
{
    game_app_t app;
}

I have a question. Why are these macros and not methods or even functions?

/* Useful macros */
#define fail_safe(x) if(!x) return;
 

 

That would make a pretty good WTF as a function ;)
void fail_safe(void* x) { if(!x) return; }
void game_app_t::display_func()
{
    fail_safe(This->m_ogldrv);
    
    /* Clear the rendering buffers */
    This->m_ogldrv->clear_buffers();
    
    /* Swap the double buffer */
    This->m_ogldrv->swap_buffers();
}

 

Be nice. I haven't C++'d since college. But I've been VB.net and C#'ing for years now :D lol

 

Though in all seriousness, shouldn't fail_safe be an exception in a function as opposed to a macro?

 

Note: I'm well aware that the OP is moving from C to C++, so idiomatic C++ is not expected from the start. But there's no harm in asking :)

0

Share this post


Link to post
Share on other sites

Wow, you guys are mean! tongue.png

 

 

I have a question. Why are these macros and not methods or even functions?

/* Useful macros */
#define safe_delete(x) if(x) { delete x; x = NULL; }
#define fail_safe(x) if(!x) return;

 

Does it matter?

 

 

Another question. Why are you using

/* Global class instance */
game_app_t* This = NULL;
 

 

and not

 

int main( int argc, char* argv[] )
{
    app = new game_app_t();
    /* where game_app_t is an actual class */
    return 0;
}
 

 

Well, you can't explicitly use "this" in a static function, hence why I created a copy of it.  I didn't feel like changing the code structure again if I didn't have to.  Second, I forgot to add the '()'.

 

Mr. Hodgeman, I'm a "heap freak". smile.png

 

Oh, and the macros were just a quick way of getting around. Second, macros can be more optimal (IMO) and for small operations I'd much rather use them to do things that require 2 lines of code than an actual short function.  Even if you inline a function that you use frequently, a macro is faster because the compiler doesn't have to push the parameters, call the function, pop them off the stack, do push the registers (eax, ebx, etc.), do whatever, restore the registers and return.  I know that it doesn't impact performance really, but after writing and reading so much assembly code, I tend to think about these things a bit too much.

 

Shogun

Edited by blueshogun96
0

Share this post


Link to post
Share on other sites

Does it matter?

 

I'll let skilled C++ programmers answer that. As I would just be saying stuff that may or may not be true.

0

Share this post


Link to post
Share on other sites

template void safe_delete(T* x) { delete x; x = NULL; }

 

This should probably be safe_delete(T*& x) as otherwise you're only assigning the local variable to NULL.

Edited by IncidentRay
2

Share this post


Link to post
Share on other sites

Wow, are you psychic or something?  How did you know I started with MSVC 6.0? dry.png   Heh, I guess when you're experienced enough, it's just obvious.

 

As you can see, I'm really old fashioned.  No one ever told me that delete NULL was valid, so I'm going to gladly take your advice.

 

Shogun.

0

Share this post


Link to post
Share on other sites

So short, so sweet.  Isn't C++ great once you use it properly?

Sure, except for you're not using it properly (to put it bluntly). Seeing as the constructor never finishes being called, and the non-trivial destructor is called (while effectively in the constructor), you've officially invoked undefined behavior. See this StackOverflow question for some details
2

Share this post


Link to post
Share on other sites

So short, so sweet.  Isn't C++ great once you use it properly?

Sure, except for you're not using it properly (to put it bluntly). Seeing as the constructor never finishes being called, and the non-trivial destructor is called (while effectively in the constructor), you've officially invoked undefined behavior. See this StackOverflow question for some details

What if I used the stack instead?

0

Share this post


Link to post
Share on other sites

What if I used the stack instead?

Nope. First, you can't delete things on the stack. And second, even if you manually called the destructor, it's still undefined behavior. If you use the non-trivial destructor in any way before returning from the constructor, it's undefined behavior. Edited by Cornstalks
1

Share this post


Link to post
Share on other sites

Wow, are you psychic or something?  How did you know I started with MSVC 6.0? dry.png   Heh, I guess when you're experienced enough, it's just obvious.

Hahahah, I didn't actually mean to level that particular accusation at you! I actually assumed you'd picked up that macro from reading tutorials etc, as it's quite common, due to MSVC 6 being very popular back in the day.
It was where I cut my teeth on C++ as well cool.png

0

Share this post


Link to post
Share on other sites

^lol, no worries.  I started C++ before .net and ended up picking up many bad programming practices.  Back then, I was very smug and thought I knew everything (mostly because I stayed in a small town in the midwest where there was no one to challenge what little skill I had, or call me out on my BS).  Even then, I had no excuse.  That what you guys are for. wink.png

 

What if I used the stack instead?

Nope. First, you can't delete things on the stack. And second, even if you manually called the destructor, it's still undefined behavior. If you use the non-trivial destructor in any way before returning from the constructor, it's undefined behavior.

Well, I'd never call delete on a stack object. >.<

 

I was thinking of changing it to this so the constructor returns and the lifecycle begins:

 

int main(...)
{
    game_app_t* app = new game_app_t();
    app->kick_off(); /* Start the main loop */

    return 0;
}

 

Shogun.

1

Share this post


Link to post
Share on other sites

 

I have a question. Why are these macros and not methods or even functions?

/* Useful macros */
#define safe_delete(x) if(x) { delete x; x = NULL; }
#define fail_safe(x) if(!x) return;

 

Does it matter?

/* I'll be clever with some temporary variables... */
Foo ** ptr = array_of_arrays_of_foo;
while(length--)
        safe_delete(*(ptr++));

safe_delete(array_of_arrays_of_foo);

Wait, why did I segfault?

0

Share this post


Link to post
Share on other sites

What does that have to do with macros and functions?



Shogun.

 

Everything. If it were a function, it'd be conceptually like this:

x = *(ptr++);
if(x){
        delete x;
        x = NULL;
}

Since it is a macro, which deals only with tokens and knows nothing about the other phases of compilation, it looks (literally) like this:

if(*(ptr++)) { delete *(ptr++); *(ptr++) = NULL; }

Notice how I only increment once in the invocation of the macro, but the token "x" is replaced with the macro parameter three times, resulting in it incrementing the pointer three separate times. This puts it past the end of the array, and causes undefined behavior.

Anybody (you, or someone else that uses your code) might make the assumption that this wouldn't happen. You might forget the body of the macro, and later on do something like I posted, which will crash the program. My policy on it is if the macro is to be used outside of that module that it was defined, it should not have any multiple-evaluation side effects; otherwise, I should provide it as a function, instead. I only use "unsafe" (in this sense) macros where I know what all of the inputs will be, and can guarantee that it will be safe.

I'm sure it's in a C/C++ FAQ somewhere, but here are a couple articles I found with a quick search:
http://www.brainbell.com/tutors/c/Advice_and_Warnings_for_C/Macros_and_Miscellaneous_Pitfalls.html
http://www.mikeash.com/pyblog/friday-qa-2010-12-31-c-macro-tips-and-tricks.html

The reason we're asking is because there are now better tools to do these things, and if you are not careful, this can come back to bite you. Also, most debuggers can't step into a macro invocation; they'll display the line where the macro was invoked, so it is up to you to figure out how the preprocessor expanded the macro, and what went wrong by hand, or dump the preprocessed output through compiler switches, and sift through it manually.

 

compiler doesn't have to push the parameters, call the function, pop them off the stack, do push the registers (eax, ebx, etc.), do whatever, restore the registers and return.


Also, I want to point out, that this is exactly what inline functions were designed to do. The body of the function is compiled "inline" where it is called. For your example, a macro and an inline function would have more or less the exact same result. Different compilers have different rules for what they will and won't inline, and how small the function must be for it to be inlined. However, for an inline function to do this task, it will do none of the things you mentioned, and essentially generate the same code that the macro would have, given that it was used correctly. It might even wind up being more efficient, by storing the result of x once and reusing it, rather than recalculating it three times, with potentially harmful side-effects. Edited by Ectara
0

Share this post


Link to post
Share on other sites

Oh please forget about such safe_delete macros. They are not safe as you still need to not forget to call them. You are still in the half C and half C++ trap.

Also that Java method of using new for everything when you could just use a normal stackvariable is more complicated, more dangerous of making errors and slower at same time.

Even if you want to use new, think about using std::unique_ptr which automagically calls delete for you.

2

Share this post


Link to post
Share on other sites

This thread is a perfect example of why only experienced programmers should learn c++.

 

Not trying to start a holy war, c++ has its place, but the assumed knowledge to prevent shooting your own foot off is pretty high. :)

0

Share this post


Link to post
Share on other sites

This thread is a perfect example of why only experienced programmers should learn c++.

 

Not trying to start a holy war, c++ has its place, but the assumed knowledge to prevent shooting your own foot off is pretty high. smile.png

I was an experienced programmer when I started learning C++ years ago.  I was never taught to use it properly as opposed to C.  Either way, I still disagree.

 

Shogun.

0

Share this post


Link to post
Share on other sites

A semi-serious issue with your code is that class objects aren't officially fully 'constructed' until you exit the constructor, so having your entire game loop within a constructor isn't a great idea - it might work, or might not. Chances are it'd probably usually work most of the time, but 'most of the time' isn't a good habit to get into with C++. smile.png 
It's like playing Russian roulette - even if you have 1000 empty chambers, and only one chamber with a bullet in it, a smart programmer will tell you that that you shouldn't have any bullets in it, and a smarter programmer will tell you that it's better not to play Russian roulette at all.
 
Your main loop, in this case, should look like this:

int main( int argc, char* argv[] )
{
    game_app_t game; //No need to use dynamic allocation at all. 
    game.do_main_loop(); //Call 'glutMainLoop();' after your constructor has fully finished.
    return 0;
}
Edited by Servant of the Lord
1

Share this post


Link to post
Share on other sites

Yes, that's exactly what I'm doing now, except I didn't remove the dynamic allocation part though (will probably do that later), but now it looks like this:

 

int main( int argc, char* argv[] )
{
    game_app_t* app = new game_app_t();
    app->kick_off();
    
    return 0;
}

 

The pointer does get deallocated upon exiting because I used a function pointer passed to atexit() to ensure everything is released.

 

If that's also a bad idea, feel free to say so.  I do appreciate everyone's advice.  I'm learning something new from every response, and that's why I'm not afraid to make dumb threads! smile.png

 

Shogun

Edited by blueshogun96
0

Share this post


Link to post
Share on other sites

If that's also a bad idea, feel free to say so.

I don't know it's a bad idea, per se, but I do think it's a pointless one.

 

By the time your atexit() function gets called, you don't need to free memory (because the OS is just about to delete your entire address space), and you probably shouldn't rely on everything being intact to perform I/O or other complex shutdown tasks.

 

The best solution is not to use glut at all, and use any one of it's myriad modern replacements - none of which turn your main loop into a tail call.

0

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  
Followers 0