Use of the override keyword to improve code quality

[hendu]

Recent compilers (gcc >= 4.7, clang >= 2.9) support the C++ override keyword.

In codebases such as irr this would really help improve the quality. Every time a new class is created, and it should override some function, this keyword would protect from typos and mismatching declarations.

See http://stackoverflow.com/questions/4976 ... -functions for an example.

[hendu]

http://sourceforge.net/p/irrlicht/patches/269/

Still working on an awk script to automatically add it.

[Nadro]

This is C++11 feature, so we can't currently support it (we still use just C++ features in Irrlicht). Anyway I hope than we will switch to C++11 soon, because this lang has some nice features eg. built-in support for threads, so it'll be nice improvment for Irrlicht.

[mikkis]

If using c++11, does it then need additional MS libraries when compiling with MS Visual Studio++ 2012 (when running on computer with W7 that doesnt have vs2012 installed)?
If compiling with codeblock+gcc, then it doenst require these libs?
And last question, does irr still work with xp?

[hendu]

As you see in the patch, the macro is only defined for supporting compilers. Earlier versions still build fine, tested GCC 4.2. Also, the patch contains no MSVC support at all currently, so MSVC just can't take advantage of it yet. (Note that MS added this keyword earlier than others, so it could be available in some earlier version than 2012.)

I finished the script, ran it on C*.h, fixed a few non-overrides and one mismatch. What remained? A 2216 line patch, and three bugs.

Script:

Code: Select all

#!/bin/sh
 
for file in "$@"; do
 
    tmp="tmp.$$"
 
    gawk 'BEGIN {
      RS=";\n"
      ORS=""
    }
    /virtual/ && ! /\{/ {
        RT=" _IRR_OVERRIDE_" RT
    }
    { print $0RT }
    ' "$file" > $tmp
 
    mv $tmp "$file"
 
    sed -i '/virtual/s@{@_IRR_OVERRIDE_ {@' "$file"
done
 

Bugs:

- getNumber is exposed, but setNumber not in ../../include/IGUITabControl.h
- setSpeed, getSpeed of attractionAffector are not exposed
- In COpenGLDriver, one draw2DImage matches nothing, and was misnamed (should've been Batch)

The last one was exactly the type this keyword was designed to catch, the first two came by accident when looking over things. Haven't yet checked whether they exist in trunk.

[hendu]

Note that while this is a C++11 feature, it is a compile-time warning, and all mentioned compilers support it in earlier C++ modes.

It does not change anything in the compiled library. It still would run on XP.

[devsh]

if we're going to use C++11 then we may as well ditch T&L only hardware support and Directx8 driver, and one of the software renderers, and OGL Version < 2.1, and include an ES 2.0 driver?

[Nadro]

Note that while this is a C++11 feature, it is a compile-time warning, and all mentioned compilers support it in earlier C++ modes.

I didn't know about it before. Thanks for clarification, at now this feature is fine for me, anyway we should w8 for Hybrid and CuteAlien.

@Devsh
I think that DirectX8 should be removed in v2.0, but there is no sense to drop T&L support in current drivers (D3D9 and OGL) without remove fixed-pipeline code and replace it by shaders like in OGLES2/D3D11 driver.

[CuteAlien]

The patch looks fine to me, we can extend it for other compilers like VS over time.

@Devsh: I'm also in favor of dropping DX8 (as I think anyone still needing that is probably supporting old applications and will be fine using old Irrlicht versions). I can't tell about Software drivers as I don't think anyone except Thomas understands the fantastic burnings renderer. So once we drop the other one we would be left with a driver where a single coder knows his way around it. And I don't even know if/how active Thomas still is.

[hybrid]

Ok, added this macro now. As it is not yet used in the code, it won't harm anyway. But of course this will only make sense if we add all those changes from the huge patch not yet published. Could you please upload the huge patch as well, hendu?
Also, we should talk about the problems mentioned above. Not sure if setNumber in the tab class shoud be exposed, nor virtual. Looks like it is only used for organizational stuff and should not be messed around.
The setSpeed/getSpeed of the affector seems useful to be exposed. Will check which changes are where.
The draw2DImage stuff is definitely one example of worst design nightmares. The interface of those functions is complicated, the naming scheme of now 3 or 4 different, but still similar functions is weird, and still I don't really know which functions are really needed still. Guess we can only patch here a little, most of the stuff needs to stay as it is. But I will fix the wrong overload.

[hendu]

That's why I posted the script that adds them. The huge patch is against my 1.7 branch, not trunk.

[hybrid]

Ok, works with my mingw shell. However, the script only hits a very small part of all those relevant functions. Seems like you match only functions which have their impleemntation in the same line (having both vurtual and curly brace in one line). However, all virtual declarations also have to be considered (now working on that) and then also those functions being placed in more than one line have to be considered (next round). Quite a number of inconsistencies already found again (mostly not exposed API functions).

[AReichl]

Cannot compile it with GCC ( > 4.7 with -std=c++11 ).

found this:
http://stackoverflow.com/questions/1277 ... turn-types

Have not tried it with Visual Studio, but i assume it works, or you would not have checked it in.

[AReichl]

ahh - 'override' does not belong in a (abstract) base class such as "CCgMaterialRenderer".

'final' can be used in base classes, 'override' in derived classes; else the logic behind it would not make sense.

[hybrid]

Hmm, CCgMaterialRenderer is no base class, as it derives from IMaterialRenderer. What errors do you get?