4
\$\begingroup\$

A similar question was asked here, and I'm trying to do the same thing. But

  1. I'm trying a different approach by deriving from std::array, and
  2. This is a very focused question about constructors only.

Here is my gl_wrap.h:

// header guards snipped

#include <array>
#include <algorithm>
#include <cstring>
#include <GL/gl.h>

namespace gl_wrap {

// aliases for style consistency and to mark as parts of gl_wrap
using gl_name = GLuint;
using gl_enum = GLenum;

template<size_t N>
class buffer_objects : public std::array<gl_name, N> {
public:
    buffer_objects() noexcept;
    ~buffer_objects();
    buffer_objects(const buffer_objects&) = delete;
    buffer_objects operator=(const buffer_objects&) = delete;
    buffer_objects(buffer_objects&& from) noexcept;
    buffer_objects& operator=(buffer_objects&& from) noexcept;
};

template<size_t N>
buffer_objects<N>::buffer_objects() noexcept
{
    glGenBuffers(N, this->data());
}

template<size_t N>
buffer_objects<N>::~buffer_objects()
{
    glDeleteBuffers(N, this->data());
}

template<size_t N>
buffer_objects<N>::buffer_objects(buffer_objects<N>&& from) noexcept
    : std::array<gl_name, N>(std::move(from))
{
    memset(from.data(), 0, N * sizeof(gl_name));
}

template<size_t N>
buffer_objects<N>& buffer_objects<N>::operator=(buffer_objects<N>&& from)
    noexcept
{
    std::array<gl_name, N>::operator=(std::move(from));
    memset(from.data(), 0, N * sizeof(gl_name));
    return *this;
}

}
// namespace gl_wrap

Some specific questions I have about this code/approach are, if applicable,

  1. Error handling design decision: should the constructors throw, or should I leave error checking, if desired, to the caller?
  2. Design decision: is it a good idea to use templates here? Will this lead to problematic code bloat if I'm using a lot of different-sized buffer_objects, or is it efficient? Can I evaluate this through profiling?
  3. Are the memcpys worth it to put the rvalues in a valid non-owning state, or can I treat the rvalues as non-owning leftovers?
  4. Am I properly using the base class's move constructor?
\$\endgroup\$
2
  • 3
    \$\begingroup\$ Note that c++ standard library classes usually aren't mean for inheritance. Better use a std::array class member variable instead. \$\endgroup\$ – πάντα ῥεῖ Aug 17 '20 at 20:30
  • 2
    \$\begingroup\$ Looks like your move semantics are doing as much work as a copy. \$\endgroup\$ – Martin York Aug 17 '20 at 21:29
5
\$\begingroup\$

Some specific questions I have about this code/approach are, if applicable,

OK.

Error handling design decision: should the constructors throw, or should I leave error checking, if desired, to the caller?

Either the object is correctly initialized and ready to use or it should throw. Two stage initialization (construct then check) is a bad idea as it leaves it open to the user to do the correct thing. You should make sure your object can not be abused not relie on the user to not abuse you.

Design decision: is it a good idea to use templates here?

If you are going to use std::array you don't really have a choice.

Will this lead to problematic code bloat if I'm using a lot of different-sized buffer_objects, or is it efficient? Can I evaluate this through profiling?

Potentially this will lead to methods be compiled for different types. But this is compiler dependent and some modern compiler can optimize this out. But what are you comparing it too?

Are the memcpys worth it to put the rvalues in a valid non-owning state, or can I treat the rvalues as non-owning leftovers?

I think you need to null out the src values. Otherwise the destructor is going to release the names. Alternatively you can store more state about if the object is valid and make the call to glDeleteBuffers() conditional on the object being in a valid state.

But I also think there is a bug here. You have just copied over the values. But you did not release the values on the dst side before the copy so you have lost the names stored there.

Remember that a std::array does not have its own move (as the data is local to the object not dynamically allocated). It moves the underlying objects between containers.

Am I properly using the base class's move constructor?

Yes it seems ligit.

Suggestions.

I would make the std::array a member rather inherit from it.

I would do:

#ifndef THORSANVIL_GL_WRAPPER_H
#define THORSANVIL_GL_WRAPPER_H

#include <GL/gl.h>
#include <array>
#include <algorithm>
#include <cstring>

namespace ThorsAnvil::GL {

template<size_t N>
class BufferObjects
{
    std::array<GLuint, N>  buffer;
    bool                   valid;
    public:
        BufferObjects() noexcept;
       ~BufferObjects();

        // Note: These are non copyable objects.
        //       Deleting the copy operations.
        BufferObjects(BufferObjects const&)           = delete;
        BufferObjects operator=(BufferObjects const&) = delete;

        // Note: The move is as expensive as a copy operation.
        //       But we are doing a logical move as you 
        //       can not have two objects with the same resource.
        BufferObjects(BufferObjects&& from)            noexcept;
        BufferObjects& operator=(BufferObjects&& from) noexcept;

        // Reset an invalid object.
        // Note: If object is valid no action.
        void reset();

};

template<size_t N>
BufferObjects<N>::BufferObjects() noexcept
    : valid(false)
{
    reset();
}

template<size_t N>
BufferObjects<N>::~BufferObjects()
{
    if (valid) {
        glDeleteBuffers(N, buffer.data());
    }
}

template<size_t N>
BufferObjects<N>::BufferObjects(BufferObjects<N>&& from) noexcept
{
    // Move the resources from the other object.
    std::move(std::begin(from.buffer), std::end(from.buffer), std::begin(buffer));

    // Maintain the correct valid states
    // The rhs is no longer in a valid state and has no resources.
    valid = from.valid;
    from.valid = false;
}

template<size_t N>
BufferObjects<N>& BufferObjects<N>::operator=(BufferObjects<N>&& from)
    noexcept
{
    // The standard copy and swap not efficient.
    // So we should do a test for self assignment
    if (this != &from)
    {
        // Destroy then re-create this object.
        // Call destructor and then placement new to use
        // the move copy constructor code to set this value.
        ~BufferObjects();
        new (this) BufferObjects(std::move(from));
    }
    return *this;
}

template<size_t N>
void BufferObjects::reset()
{
    if (!valid) {
        glGenBuffers(N, buffer.data());
        valid = true;
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.