A Challenge in C++

Graphics.cpp lines 117 to 120:

Code:

		do
		{
			retVal = window.CheckMessages();
		} while ((clock() - frameStart) < (1000 / FPS) && window.state == APPSTATE_RUNNING);

Consider using CLOCKS_PER_SEC for greater portability.

[edit] And, because I like enum, I'd suggest using one here (Graphics.h lines 3-11):

Code:

#ifndef APPSTATES
#define APPSTATES
#define APPSTATE_UNINITIALIZED	0
#define APPSTATE_INITIALIZED	1
#define APPSTATE_RUNNING		2
#define APPSTATE_DESTROYING		3
#define APPSTATE_DESTROYED		4
#define APPSTATE_PAUSED			5
#endif

Mainly because you wouldn't have to keep track of the numbers.

Code:

#ifndef APPSTATES
#define APPSTATES

enum {
    APPSTATE_UNINITIALIZED,
    APPSTATE_INITIALIZED,
    APPSTATE_RUNNING,
    APPSTATE_DESTROYING,
    APPSTATE_DESTROYED,
    APPSTATE_PAUSED
};

#endif

And then, of course, instead of this (Graphics.h line 18):

Code:

unsigned state;

You could use this (assuming you named the enum):

Code:

enum appstate {
    /* ... */
};

/* ... */

appstate state;

In C++, you can't actually assign any old integer to an enum (without a cast, anyway), so you get extra type-safety. You won't assign 18 by accident, which isn't an APPSTATE, of course.

[edit=2]
Also, this looks weird, but I guess it works.

Code:

displayX = (int)(short)LOWORD(lParam);

[/edit]

[edit=3]
From lines 13-27 of Application.cpp:

Code:

unsigned CApplication::Run()
{
	int retVal;

	state = APPSTATE_RUNNING;

	retVal = graphics.Run();

	state = APPSTATE_DESTROYING;

	graphics.Destroy();

	state = APPSTATE_DESTROYED;
	return retVal;
}

You're storing the unsigned result of graphics.Run() in a signed int; then you return that from an unsigned function; then you return that as a signed int from main(). Perhaps it would make more sense to make up a standard, like only signed ints? [/edit]

Consider using CLOCKS_PER_SEC for greater portability.

Excellent idea

And, because I like enum, I'd suggest using one here

I'm taking C++ over C every chance I get and I agree with your reasons, so I'll change my code.

Also, this looks weird, but I guess it works.

Straight from good old msdn.

Perhaps it would make more sense to make up a standard, like only signed ints?

I was returning all ints until I learned that wParam was an unsigned int. I thought I had everything changed.
Thanks for all the suggestions.

Actually, don't use clock() for timing at all. It times the CPU time - it might not even pick up time spent in GFX calls. It definitely doesn't pick up time spent waiting. If your message loop idles even for a bit, the timing will be off. Also, the precision is not all that great.

I'd recommend timeGetTime or QueryPerformanceCounter, but of course neither is portable. You should factor this little detail out into a function that you can give a platform-specific implementation. Or you could search for cross-platform precision timers.

It times the CPU time - it might not even pick up time spent in GFX calls. It definitely doesn't pick up time spent waiting. If your message loop idles even for a bit, the timing will be off. Also, the precision is not all that great.

Well, I don't want that now do I. I switched to timeGetTime. I'm not worried about cross-platform because I'm going to be using windows' threads anyway.

Also, this looks weird, but I guess it works.

Code:

displayX = (int)(short)LOWORD(lParam);

Straight from good old msdn.

The reason I mention it is that because displayX is an int, a short will automatically be promoted to an int, even if you left out the (int) cast. This would work just fine.

Code:

displayX = (short)LOWORD(lParam);

Also, since you're "taking C++ over C every chance [you] get", you could use a static_cast instead of a C-style cast there.

Code:

displayX = static_cast<short>(LOWORD(lParam));

Since Window.h uses HWND and HINSTANCE and other types from <windows.h>, I'd include windows.h in there. It's not a good idea to assume that whatever function is using a header file will have included some other header file beforehand.

Actually, don't use clock() for timing at all. It times the CPU time - it might not even pick up time spent in GFX calls. It definitely doesn't pick up time spent waiting. If your message loop idles even for a bit, the timing will be off. Also, the precision is not all that great.

Using time() instead of clock() would solve all of those problems save the last.

I'd recommend timeGetTime or QueryPerformanceCounter, but of course neither is portable. You should factor this little detail out into a function that you can give a platform-specific implementation. Or you could search for cross-platform precision timers.

QueryPerformanceCounter() is a Windows-specific function, it's true, but that code includes a lot of other Windows-specific code with no concern for portablility, so there's probably nothing wrong with using it.

Also, in CWindow::WindowProc(), you're returning both 0 and true as if they were LRESULTs. I'd stick with true and false or 1 and 0, but that's just me.

You could also perhaps make CGraphics::Destroy() into the CGraphics destructor, but I can understand your wanting to be symmetrical with Initialize(), which returns a value. If you wanted to make both of those functions into constructors and destructors, you could throw an exception from the constructor to indicate failure instead of returning false. Just a thought.

And you don't really need multiple inclusion guards in your header files, like CGRAPHICS and APPSTATES in Graphics.h -- a since #ifdef around the entire code would suffice. Unless you want to be able to include only part of the header file, but I don't see why you'd want to do that.

The reason I mention it is that because displayX is an int, a short will automatically be promoted to an int, even if you left out the (int) cast. This would work just fine.
Also, since you're "taking C++ over C every chance [you] get", you could use a static_cast instead of a C-style cast there.

Good point about the short being promoted to an int and I've already changed all my casts to C++ style

Since Window.h uses HWND and HINSTANCE and other types from <windows.h>, I'd include windows.h in there. It's not a good idea to assume that whatever function is using a header file will have included some other header file beforehand.

I moved alot of code around and I need to go back and check for things like that. I didn't intend to assume it was already included.

Also, in CWindow::WindowProc(), you're returning both 0 and true as if they were LRESULTs. I'd stick with true and false or 1 and 0, but that's just me.

I blame MSDN again. In the remarks, it says what the return value should be if the messsage is handled properly. Most say to return 0 but that one said to return true.

You could also perhaps make CGraphics::Destroy() into the CGraphics destructor

I'm trying to destroy things in a certain order so I need extra control. I can't assume that the classes will go out of scope at the right times.

Unless you want to be able to include only part of the header file, but I don't see why you'd want to do that

I defined APPSTATES it in multiple headers so a cpp file could have picked it up from another header file and doesn't need it from the header file it is currently looking at. At least that was my logic. I assume that at least some of my logic or at least methods are wrong.

QueryPerformanceCounter() is a Windows-specific function, it's true, but that code includes a lot of other Windows-specific code with no concern for portablility, so there's probably nothing wrong with using it.

I still haven't decided what to use. Do you recommend QueryPerformanceCounter() over the other options?

Thanks again for your advice

I defined APPSTATES it in multiple headers so a cpp file could have picked it up from another header file and doesn't need it from the header file it is currently looking at. At least that was my logic. I assume that at least some of my logic or at least methods are wrong.

You probably shouldn't be defining APPSTATES more than once. I'd put the APPSTATES code in yet another header file, and include that wherever you need it.

I still haven't decided what to use. Do you recommend QueryPerformanceCounter() over the other options?

Probably, since your code looks Windows-only. You get much more accuracy (or is it precision?) with it, compared with portable functions like time().

Also, memset(), which you use at Graphics.cpp line 32 (in CGraphics::Initialize()), and perhaps elsewhere, is in <cstring>. http://www.opengroup.org/onlinepubs/...sh/memset.html

Don't forget that it would be std::memset() unless you have some using directives of some sort.

Originally Posted by dwks

Don't forget that it would be std::memset() unless you have some using directives of some sort.

I get the error that memset is not a member of std. I think it's one of three things:
1. I didn't include the right header file. It possible but I haven't found any information on a C++ header containing memset that doesn't start with the letter c. ex <cstring>
2. My compiler is out-dated. I'm using MSVC 6.0 since I couldn't get 2005 Express to work on my 64bit vista machine (I've already had to download updated versions of a few header files)
3. memset is simply not a member of std and there is another function I should be using. (I couldn't get fill() to work). Of course I have faith that you know more than me so this idea is a stretch.

I think it's option 2.
1. Possible, but non-portable. memset() should be in <cstring.h>, as a simple google search will show.
2. Most likely it's your compiler. BTW, I've had troubles with 64-bit Vista machines, too.
3. You could use another function if you wanted to -- memset() is a standard C function, after all. I don't know of any offhand.

The nice thing about memset() is that some compilers (GCC comes to mind) will actually implement it as an inline assembler loop, if you turn on optimisations. This means that it will probably be faster than anything else you can come up with.

If you include <string.h> (the actual C header file), you don't have to prefix std::. But I think that may be deprecated, or at least un-C++-ish.

To get Dev-C++ to work under Vista (it was 64-bit, I don't know if that matters), I had to write a sort of patch for it, because gcc would only execute with the current directory in bin/. I added it to autoexec.bat and everything. (Though now that I think about it, perhaps Vista doesn't execute autoexec.bat, and I should have used the Control Panel to add Dev-C++'s GCC to the path.)

You could always try Cygwin, if you have a very fast internet connection and want a Windows bash implementation to play around with. I've downloaded it and will try it myself sometime on the Vista machine.