Call a derived class' (non-virtual) function in the base class' Destructor-CodePudding

Suppose we have the following class template:

template<typename T>
class Object
{
public:
    Object() = default;
    Object(const Object&) = delete;
    Object(Object&& other) noexcept
    {
        if (this != &other)
        {
            static_cast<T*>(this)->Release();
            m_Id = std::exchange(other.m_Id, 0);
        }
    }

    auto operator=(const Object&) = delete;
    Object& operator=(Object&& other) noexcept
    {
        if (this != &other) {
            static_cast<T*>(this)->Release();
            m_Id = std::exchange(other.m_Id, 0);
        }

        return *this;
    }

    ~Object()
    {
        static_cast<T*>(this)->Release();
        m_Id = 0;
    }

protected:
    std::uint32_t m_Id;
};

(Please ignore the duplication in the move constructor and move assignment operator for the moment)

This class is meant to act as a base class for OpenGL object wrappers. Following is an example use:

class VertexBuffer : public Object<VertexBuffer>
{
public:
    VertexBuffer()
    {
        glGenBuffers(1, &m_Id);
        ...
    }

    void Release()
    {
        glDeleteBuffers(1, &m_Id);
    }
};

The Object<T> class template is supposed to take care of the bookkeeping.

The reason for doing this is that the pattern in the Object<T> class is repeated the exact same way for (almost) every OpenGL object wrapper that might be written. The only difference is the creation and deletion of the objects which is handled by the constructor and the Release() function in this example.

Now the question is whether this (Object<T>::~Object() to be specific) taps into UB land? Undefined Behavior Sanitizer doesn't report any errors but I've never done this, so I though of asking people with more experience to make sure.

CodePudding user response：

Don't do that. That'll cause an undefined behavior.

Instead, implement the template class as a derived class, like the following example.

class BufferGrandBase {
protected:
    GLuint id;
};

template<class B>
class Buffer : public B {
public:
    Buffer() {
        B::Create();
    }
    ~Buffer() {
        B::Destroy();
    }
};

class VertexBufferBase : public BufferGrandBase {
public:
    void Create() { glGenBuffers(1, &id); }
    void Destroy() { glDeleteBuffers(1, &id); }
};
typedef Buffer<VertexBufferBase> VertexBuffer;

This pattern will also simplify implementing constructors and operators.

CodePudding user response：

Short answer: Yes, this is undefined behaviour, don't do that.

Long answer:
The destruction of VertexBuffer invokes first ~VertexBuffer() and then invokes ~Object<VertexBuffer>() afterwards. When ~Object<VertexBuffer>() is invoked the VertexBuffer "part" of the object has already been destroyed, i.e. you are now doing an illegal downcast via static_cast (the remaining valid part of the object is a Object<VertexBuffer>, not a VertexBuffer).

And undefined behaviour permits the compiler to do ANYTHING - it might even (appear to) work, only to suddenly stop working (or only work when you build in Debug mode, but not when in Release). So, for your own sake, please don't do that.

CodePudding user response：

If you have a "thing" that holds an Object<T> where T is the Crtp-type, that "thing" is likely a template anyway.

So instead of holding an Object<T>, why don't you just hold a T, which is the full type that inherits from Object<T>. If that is destroyed it will call T::~T() automatically.

In addition, perhaps you want to do private or protected inheritance from Object<T> to discourage users from slicing the Crtp-type.