Don’t point, it’s impolite

14 08 2010

Before starting this I’ll post some code that gave me the headache.

It isn’t complex code, just a simple Win32 window class wrapper for the [surprise, surprise] window using WIN32 API.
class Win32
{
  public:
    void Create(std::string name);
    void Destroy();
  private:
    WNDCLASSEX mWindowClass;
    HINSTANCE mInstanceHandle;
};

void Win32::Create(std::string name)
{
  mWindowClass.cbSize = sizeof(WNDCLASSEX);
  mWindowClass.style = CS_HREDRAW | CS_VREDRAW | CS_OWNDC;
  mWindowClass.lpfnWndProc = Tool::WinProc;
  mWindowClass.cbClsExtra = 0;
  mWindowClass.cbWndExtra = 0;
  mWindowClass.hInstance = mInstanceHandle;
  mWindowClass.hIcon = LoadIcon(nullptr, IDI_APPLICATION);
  mWindowClass.hCursor = LoadCursor(nullptr, IDC_ARROW);
  mWindowClass.hbrBackground = (HBRUSH)(COLOR_WINDOW+1);
  mWindowClass.lpszMenuName = nullptr;
  mWindowClass.lpszClassName = name.c_str();
  mWindowClass.hIconSm = LoadIcon(nullptr, IDI_APPLICATION);
  RegisterClassEx(&mWindowClass);
}

void Win32::Destroy()
{
  UnregisterClass(mWindowClass.lpszClassName, mInstanceHandle);
}

I’ve omitted some tests and some other bits which aren’t really important for this post. But the gist of it is there, and it’s mostly fairly trivial.
Can you stop my one, HUGE, mistake there?

Run this code and watch it go up in smoke. The UnregisterClass() function in the Destroy() method will break and quite badly, in fact when I ran it, it reported that the class in question doesn’t even exist. But, but, I’ve registered it, I know I did and the RegisterClass() function worked quite fine, nothing broken there, so, what’s the problem??

A bit of debugging later and I found that the mWindowClass.lpszClassName in the Destroy function contains garbage. Apparently somewhere betweeen calling Create() and Destroy() something bad happened to the lpszClassName [have I mentioned how much I dislike this naming convention??]

Bit more of debugging, running through the breakpoints and examining the contents of the memory and…

Pointers…. Who the hell had the brilliant idea to use a naked char* for the window class name in the WNDCLASSEX [that’s WIN32 API for you Linux, Apple, or other OS people other there].

That should’ve given you the answer by now, if not, then here it goes.

When we call the Create function it generates the name string temporary on the stack and continues on. Then at some later point we’re assigning the name.c_str() to the mWindowClass.lpszClassName – this is a pointer, it’s not a true assignment of values but rather the char* is now pointing to the beginning of the temporary array of characters that is name. Everything works fine inside the Create, we finish what we have to do and leave the function. Woosh! The function temporaries [being temporary] get deleted, erased and/or filled with garbage and the mWindowClass.lpszClassName is now pointing at, GARBAGE!

Yey, found the bug; argh, should have been more careful with my pointers. Fixing it isn’t hard, just introduce a string variable in the Win32 class to hold the name through the life-duration of the class itself.

I should have done it this way originally but I thought I’d be smart about things and save some memory. The moral of the story is, as usual, code correctly, make sure everything is expressed properly and stop trying to be too smart for my own good.

It’s been a long December and there’s reason to believe next year will be better than the last…

Advertisements

Actions

Information

2 responses

14 08 2010
Tate

You could also create a static variable within the function itself, to hold the string. You won’t have as much control over when you release the string that way, but it’s just a few characters and unlikely to be the biggest memory issue in a PC game

14 08 2010
shattenyagger

yeah, but sprinkling statics isn’t something I like doing, even [or especially] for trivial things, like you said, it also means less control over the memory. In this case I wanted to expose the name for reading by the user anyway. I actually didn’t need to keep a record of the mWindowClass. All I really need is the name and with it I can retrieve the WNDCLASSEX information using GetClassInfoEx().

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s




%d bloggers like this: