Use the deferred virtual call mechanism to avoid the virtual
call bug in the constructor. See docs/wrong_virtual_call.md for a description.
This commit is contained in:
parent
0b6cdeb5f5
commit
5e76b49cae
6 changed files with 64 additions and 11 deletions
25
docs/wrong_virtual_call.md
Normal file
25
docs/wrong_virtual_call.md
Normal file
|
@ -0,0 +1,25 @@
|
|||
1) SizeNotifiable, defined as the following:
|
||||
|
||||
class SizeNotifiable<RegisterBehaviour, true> : private RegisterBehaviour, public SizeNotifiableBase { ... };
|
||||
|
||||
gets constructed - it inherits from SizeNotifiableBase which declares NotifyResChanged() as virtual. It also inherits from RegisterBehaviour, which must provide a Register() method.
|
||||
|
||||
2) In SizeNotifiable's constructor RegisterBehaviour::Register(this) gets called. Being in the constructor, virtual calls on this are UB.
|
||||
|
||||
3) When RegisterBehaviour == AutoRegister, we have Register defined as the following:
|
||||
|
||||
void AutoRegister::Register (SizeNotifiableBase* parNotifiable) {
|
||||
m_id = m_sdlmain->RegisterForResChange(parNotifiable);
|
||||
}
|
||||
|
||||
note that parNotifiable is received from SizeNotifiable's constructor, so it's a partially constructed object.
|
||||
|
||||
4) RegisterForResChange() is defined as:
|
||||
|
||||
size_t SDLMain::RegisterForResChange (SizeNotifiableBase* parNotif) {
|
||||
parNotif->NotifyResChanged(m_localData->sizeratio);
|
||||
return m_localData->resChangeNotifList.Add(parNotif);
|
||||
}
|
||||
|
||||
parNotif is, again, the partly constructed object pointed by this in SizeNotifiable's constructor. NotifyResChanged() is a virtual method, thus the bug.
|
||||
Objects being registered need to be notified straight away since they have no clue about the current size held by SDLMain.
|
|
@ -43,7 +43,7 @@ namespace cloonel {
|
|||
Placeable(float2(0.0f)),
|
||||
Drawable(parSize),
|
||||
m_bottomBar(float2(0.0f), parSize.x()),
|
||||
m_screenRatio(parMain),
|
||||
m_screenRatio(parMain, DeferredRegister(&m_screenRatio)),
|
||||
m_bounceCallback(&DoNothing),
|
||||
m_texture(new Texture(parPath, parMain, false))
|
||||
#if defined(WITH_DEBUG_VISUALS)
|
||||
|
|
|
@ -28,7 +28,7 @@ namespace cloonel {
|
|||
///--------------------------------------------------------------------------
|
||||
Platform::Platform (SDLMain* parSdlMain, const float2& parPos, Texture* parTexture, const float2& parSize) :
|
||||
Placeable(parPos),
|
||||
m_screenRatio(parSdlMain),
|
||||
m_screenRatio(parSdlMain, &m_screenRatio),
|
||||
m_size(parSize),
|
||||
m_collisionTop(new HorzCollisionBar(parPos, parSize.x())),
|
||||
m_surface(parTexture)
|
||||
|
|
|
@ -1,8 +1,8 @@
|
|||
#include "sizenotifiable.hpp"
|
||||
#include "sizeratio.hpp"
|
||||
#include "sdlmain.hpp"
|
||||
#include <cassert>
|
||||
#include <ciso646>
|
||||
#include <exception>
|
||||
|
||||
namespace cloonel {
|
||||
namespace implem {
|
||||
|
@ -62,4 +62,12 @@ namespace cloonel {
|
|||
///--------------------------------------------------------------------------
|
||||
void SizeNotifiableBase::OnResChanged (const SizeRatio&) {
|
||||
}
|
||||
|
||||
///--------------------------------------------------------------------------
|
||||
///--------------------------------------------------------------------------
|
||||
DeferredRegister::~DeferredRegister() {
|
||||
assert(m_call);
|
||||
if (not std::uncaught_exception())
|
||||
m_call();
|
||||
}
|
||||
} //namespace cloonel
|
||||
|
|
|
@ -24,6 +24,7 @@
|
|||
#include <cstddef>
|
||||
#include <algorithm>
|
||||
#include <functional>
|
||||
#include <cassert>
|
||||
|
||||
namespace cloonel {
|
||||
class SizeRatio;
|
||||
|
@ -69,6 +70,19 @@ namespace cloonel {
|
|||
};
|
||||
} //namespace regbehaviours
|
||||
|
||||
template <class RegisterBehaviour, bool=RegisterBehaviour::SDLMAIN_NEEDED>
|
||||
class SizeNotifiable;
|
||||
|
||||
class DeferredRegister {
|
||||
public:
|
||||
template <typename R>
|
||||
DeferredRegister ( SizeNotifiable<R>* parObject ) noexcept;
|
||||
~DeferredRegister ( void );
|
||||
|
||||
private:
|
||||
std::function<void()> m_call;
|
||||
};
|
||||
|
||||
class SizeNotifiableBase {
|
||||
protected:
|
||||
SizeNotifiableBase ( void ) = default;
|
||||
|
@ -86,12 +100,10 @@ namespace cloonel {
|
|||
float2 m_scaleRatio;
|
||||
};
|
||||
|
||||
template <class RegisterBehaviour, bool=RegisterBehaviour::SDLMAIN_NEEDED>
|
||||
class SizeNotifiable;
|
||||
|
||||
template <class RegisterBehaviour>
|
||||
class SizeNotifiable<RegisterBehaviour, false> : private RegisterBehaviour, public SizeNotifiableBase {
|
||||
static_assert(RegisterBehaviour::SDLMAIN_NEEDED == false, "SdlMainNeeded mismatches expected value");
|
||||
friend class DeferredRegister;
|
||||
public:
|
||||
SizeNotifiable ( const SizeNotifiable& ) = delete;
|
||||
SizeNotifiable ( SizeNotifiable&& parOther ) :
|
||||
|
@ -99,8 +111,8 @@ namespace cloonel {
|
|||
SizeNotifiableBase(parOther)
|
||||
{
|
||||
}
|
||||
SizeNotifiable ( void ) {
|
||||
this->Register(this);
|
||||
explicit SizeNotifiable ( const DeferredRegister& parRegisterFunctor ) {
|
||||
static_cast<void>(parRegisterFunctor);
|
||||
}
|
||||
virtual ~SizeNotifiable ( void ) noexcept {
|
||||
this->Unregister();
|
||||
|
@ -110,6 +122,7 @@ namespace cloonel {
|
|||
template <class RegisterBehaviour>
|
||||
class SizeNotifiable<RegisterBehaviour, true> : private RegisterBehaviour, public SizeNotifiableBase {
|
||||
static_assert(RegisterBehaviour::SDLMAIN_NEEDED == true, "SdlMainNeeded mismatches expected value");
|
||||
friend class DeferredRegister;
|
||||
public:
|
||||
SizeNotifiable ( const SizeNotifiable& ) = delete;
|
||||
SizeNotifiable ( SizeNotifiable&& parOther ) :
|
||||
|
@ -117,15 +130,22 @@ namespace cloonel {
|
|||
SizeNotifiableBase(parOther)
|
||||
{
|
||||
}
|
||||
explicit SizeNotifiable ( SDLMain* parSdlMain ) :
|
||||
SizeNotifiable ( SDLMain* parSdlMain, const DeferredRegister& parRegisterFunctor ) :
|
||||
RegisterBehaviour(parSdlMain)
|
||||
{
|
||||
this->Register(this);
|
||||
static_cast<void>(parRegisterFunctor);
|
||||
}
|
||||
virtual ~SizeNotifiable ( void ) noexcept {
|
||||
this->Unregister();
|
||||
}
|
||||
};
|
||||
|
||||
template <typename R>
|
||||
DeferredRegister::DeferredRegister (SizeNotifiable<R>* parObject) noexcept :
|
||||
m_call(std::bind(&R::Register, static_cast<R*>(parObject), parObject))
|
||||
{
|
||||
assert(m_call);
|
||||
}
|
||||
} //namespace cloonel
|
||||
|
||||
#endif
|
||||
|
|
|
@ -98,7 +98,7 @@ namespace cloonel {
|
|||
///--------------------------------------------------------------------------
|
||||
///--------------------------------------------------------------------------
|
||||
TiledWallpaper::TileCountNotifiable::TileCountNotifiable (SDLMain* parMain, const ushort2& parTileSize) :
|
||||
BaseClass(parMain),
|
||||
BaseClass(parMain, this),
|
||||
m_tileCount(CountTilesInScreen(parMain->WidthHeight(), parTileSize)),
|
||||
m_tileSize(vector_cast<float2>(parTileSize))
|
||||
#if defined(WITH_DEBUG_VISUALS)
|
||||
|
|
Loading…
Reference in a new issue