I was looking around the ResourceManager class, and I find a nasty <potential> problem:
There's a call to RemoveAll function, which is described as "virtual". Not any virtual function should be call in ctor/dtor, as vtable does not exist yet/anymore.
In my opinion, set RemoveAll as a virtual is an API error...
There's only one override of this function, in CompositorManager (and the 'virtual' keyword was forgotten there :/).
Call to a virtual function in ResourceManager dtor
-
- Gremlin
- Posts: 193
- Joined: Fri Jan 25, 2008 6:55 pm
- Location: Nantes / France
- x 17
-
- OGRE Expert User
- Posts: 1920
- Joined: Sun Feb 19, 2012 9:24 pm
- Location: Russia
- x 201
Re: Call to a virtual function in ResourceManager dtor
Calling a virtual function in a destructor is harmless unless it's pure virtual. But it might not work as some might expect - that's true and that could be problematic.
ResourceManager having virtual removeAll() is absolutely legit since ResourceGroupManager::shutdownAll() uses it and CompositorManager needs a chance to clean up as part of that process. I suggest to leave it virtual but replace the call to it from ~ResourceManager() with an assert that would check that removeAll() has in fact been executed before destroying the ResourceManager instance. The ResourceManager clients might need to be updated accordingly - I haven't looked at that.
As for not repeating "virtual" - you only need that in the first declaration. I personally prefer not repeating that in overrides because I can then clearly see where the original virtual method is declared and where it's overriden. C++11 finally comes to the rescue with the new 'override' keyword, but I've seen some developers using an OVERRIDE macro that would expand into nothing just for the purpose of marking overrides. It might be a good approach now as you can simply define it as "override" for the C++11 capable compilers.
ResourceManager having virtual removeAll() is absolutely legit since ResourceGroupManager::shutdownAll() uses it and CompositorManager needs a chance to clean up as part of that process. I suggest to leave it virtual but replace the call to it from ~ResourceManager() with an assert that would check that removeAll() has in fact been executed before destroying the ResourceManager instance. The ResourceManager clients might need to be updated accordingly - I haven't looked at that.
As for not repeating "virtual" - you only need that in the first declaration. I personally prefer not repeating that in overrides because I can then clearly see where the original virtual method is declared and where it's overriden. C++11 finally comes to the rescue with the new 'override' keyword, but I've seen some developers using an OVERRIDE macro that would expand into nothing just for the purpose of marking overrides. It might be a good approach now as you can simply define it as "override" for the C++11 capable compilers.