Call to a virtual function in ResourceManager dtor

Minor issues with the Ogre API that can be trivial to fix
Post Reply
Nodrev
Gremlin
Posts: 193
Joined: Fri Jan 25, 2008 6:55 pm
Location: Nantes / France
x 17

Call to a virtual function in ResourceManager dtor

Post by Nodrev »

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 :/).
bstone
OGRE Expert User
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

Post by bstone »

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.
Post Reply