Code Quality

While recently working on the QuickGUI code, I found some mildly irritating stuff, some badly irritating stuff and some more stuff, that I consider highly precarious.

1 (highly precarious). There are some functions, which are not exception-safe. Not only are they leaking, some even can cause undefined behaviour.

Here is an example:


delete mWidgetImage;
mWidgetImage = new Ogre::Image(i);


What happends, if new Ogre::Image(i) throws? mWidgetImage has already been deleted, but the pointer still points at the deleted image and in the case of an exception the assignment (mWidgetImage = ) is not going to happen.

I have seen some other types of exception problems in various constructors. This needs to be reviewed sooner or later.

2 (badly irritating). The constness of various Wiidget functions is wrong.

3 (mildly irritating) Calling clear (STL-container function) on a member in the destructor is pointless, since this is done automatically, when the container is destroyed.


I can go over problem 2 next week and make another patch (should only take a few minutes), but right now I don't have the time to work on problem 1.

P.S.: I would like to make another request. Could we remove the Ogre.h include instruction from QuickGUI headers? It makes compiling awfully slow and this header isn't meant to be used in real code anyway (beyond simple demos).

Thanks for the input. I will remove mWidgetImage from the Widget class, so that code will not be used in the future. The Image is used for transparency picking. In the future I'll use the SkinSet class to tell me if I'm over a transparent pixel. With the current method it also requires all the texture images to be in Ogre resource paths, which is not desirable.

For 3, its not really a lack of knowledge, I do it because its a habit. Regardless of being in a destructor, whenever I cleanup a container my fingers just type everything out. I wonder if there is any kind of performance hit for the redundancy of clearing the container.

For 3, its not really a lack of knowledge, I do it because its a habit. Regardless of being in a destructor, whenever I cleanup a container my fingers just type everything out. I wonder if there is any kind of performance hit for the redundancy of clearing the container.
Most likely not, nothing noticable, anyway. The containers's destructors will probably recall clear or its internal implementation, but they won't do much now that you already cleared them. A function call and a few assignments, I suppose. You'd probably have to delete millions of containers that way to measure it
Mostly it just clutters your own code, so I'd still try to get rid of the habit, hehe.

I forgot to respond about your request. I plan to revamp all the files and include only what is necessary, and do as much forward declaration in header files as possible. After this is done you shouldn't see any includes to ogre headers in a QuickGUI header file.

Forgot to respond, too.

Actually including an Ogre header where it is needed is not a big problem, even in a QuickGUI header. Isolating the QuickGUI interface totally from the Ogre headers would be overkill. If you can just get rid if this ugly #include "Ogre.h" I will be happy.

Anyway, here is the promised constness patch:

http://www.zpages.de/public/Const.patch

Unfortunately it got a bit bigger than I expected (the const problems were not limited to the widget class).

Thanks for the patch! There may be a little delay in committing it, my new place doesn't have internet yet.

Finally got around to applying the patch. Thanks a lot for going through the functions. I have a noob question. A function that returns a const variable is returning by reference, right? (So basically its reducing creating a copy of the data?)

const type& someFunction();

return by reference.



const type someFunction();

return by value (copy).


The second version is only used in rare cases.

Just to jump in on random code improvement things, I recently bought a C++ book to make sure I've got the right foundation, so I'll post some observations when I have them.

Right now, the only thing I wanted to say is that according to the book,

#define VARIABLENAME <value>

statements should actually be written as

const int VARIABLENAME = <value>;

Using #define for global variables is the old C way of doing things, but that's not standard recommended practice in C++ anymore.

Amen!

haha. Noted. I don't really see anything wrong with the #define's, but I'll try to break my habits I learned when I started coding.

haha. Noted. I don't really see anything wrong with the #define's.

Then you should look closer. Lets assume QuickGUI defines a macro named SOMEMACRO.
Lets further assume we have a library X, which defines a macro named SOMEMACRO, too.

Result: You can't use both libraries in one program, because the macros are colliding.

Yeah, although that's a bad example because if the other library defines that macro, then having QuickGUI using a const with that name doesn't really improve the situation much
The actual motivation should be that QuickGUI does not become that library which pollutes the environment with names some other programmer wishes to use in his project. consts can be enclosed in namespaces, #defines cannot.

haha, good call. I'm glad to be involved with the Ogre community, I keep learning better coding practices all the time. Slowly but surely!

Then you should look closer.

Maybe I should get an eye exam.