Use of external memory or a custom allocator

Question

I'm creating a class which uses a custom buffer. I want to offer the possibility to pass an external memory address (for higher interoperability between languages) or (for convenience) to specify a custom allocator type. The following code outlines what I mean:

template<typename int_type, class alloc = void>
class uses_custom_buffers
    : uses_custom_buffers<int_type, void>
{
public:
    uses_custom_buffers<int_type, alloc>* set_buffer(std::size_t count, std::size_t size)
    {
        typename alloc::rebind<int_type*>::other pointer_alloc;
        int_type** buffer = pointer_alloc.allocate(count);
        for (std::size_t i = 0; i < count; ++i)
            buffer[i] = this->m_alloc.allocate(size);
        this->uses_custom_buffers<int_type, void>::set_buffer(count, buffer, size);
        return this;
    }

private:
    using uses_custom_buffers<int_type, void>::set_buffer;

    alloc m_alloc;
};

template<typename int_type>
class uses_custom_buffers<int_type, void>
{
public:
    uses_custom_buffers<int_type, void>* set_buffer(std::size_t count, int_type** buffers, std::size_t size)
    {
        this->m_buf   = buffers;
        this->m_count = count;
        this->m_size  = size;
        return this;
    }

private:
    int_type**  m_buf;
    std::size_t m_count,
                m_size;
};

Please note: This example doesn't care about deallocating any resource or exception safeness (to simplify matters). Do you see any kind of problems with that design?

Answer 1

I think that in order to be a good review candidate, this should be more than an "outline." Like, there should be an example of how you intend to use it.

But here's some stylistic feedback:

• CamelCase your template parameter names: IntType (or just T), Alloc.
• You can (and should) use the injected class-name uses_custom_buffers inside the class itself, rather than typing out uses_custom_buffers<int_type, alloc> every time.
• Don't define multiple (member) variables on the same line.
• Mark your constructors explicit unless you want to enable the implicit conversion for some specific reason.
• Explicitly mark your base classes public and private, for clarity.
• Writing std::size_t instead of size_t, or typename instead of class, is just extra keyboard practice. Personally, I always go for the shorter versions.
• Always brace your if and for bodies. Don't goto fail!

---

    this->uses_custom_buffers<int_type, void>::set_buffer(count, buffer, size);

This seems complicated. Ideally we'd just write

    set_buffer(count, buffer, size);

But since that set_buffer is located in a dependent base class, we actually have to write this-> in front:

    this->set_buffer(count, buffer, size);

And then we still have trouble, because the declaration of set_buffer in uses_custom_buffers<int_type, alloc> hides the declaration of set_buffer in uses_custom_buffers<int_type, void>! There are two clean ways to fix this, depending on what you want to do. The first is to bring the hidden set_buffer back into scope with a using-declaration:

using uses_custom_buffers<int_type, void>::set_buffer;

The second is to pick a different name for one or both of the functions in this overload set.

"If you have two [functions] that are doing something very very different, please, name them differently." —Titus Winters, 2018

---

I would even argue that uses_custom_buffers<int_type, void> is doing something "very very different" from uses_custom_buffers<int_type, alloc>, and therefore it should be named differently.

---

Very important:

typename alloc::rebind<int_type*>::other pointer_alloc;

This is (A) missing a template keyword, and (B) waaay too complicated for one line of code. Break it down by using a typedef:

using PointerAlloc = typename alloc::template rebind<int_type *>::other;
PointerAlloc pointer_alloc;

However, this is (C) still broken, because it fails to use allocator_traits. You need to write this instead:

using PointerTraits = typename std::allocator_traits<alloc>::template rebind_traits<int_type *>;
using PointerAlloc = typename std::allocator_traits<alloc>::template rebind_alloc<int_type *>;
PointerAlloc pointer_alloc;

And then on the next line:

int_type** buffer = pointer_alloc.allocate(count);

should be

int_type **buffer = PointerTraits::allocate(pointer_alloc, count);

However, after all that, I am still super duper confused about where your allocator is supposed to come from! You just default-constructed it and immediately used it to allocate some memory? Where is the memory supposed to come from?

If you want to use the standard allocator model, you need to provide a way for the user to pass in an allocator for you to use. You can't just default-construct one and expect it to magically know where its heap is located. (That happens to work for std::allocator because it just uses new and delete, which are global; but it is highly unlikely to work for any user-provided allocator type.)

---

Let's put it all together and see how it looks:

template<class T>
class use_custom_buffers_base {
public:
    use_custom_buffers_base *set_buffers(size_t count, T **buffers, size_t size) {
        this->m_buf   = buffers;
        this->m_count = count;
        this->m_size  = size;
        return this;
    }

private:
    T **m_buf;
    size_t m_count;
    size_t m_size;
};

template<class T, class Alloc>
class use_custom_buffers : private use_custom_buffers_base<T>
{
    using Base = use_custom_buffers_base<T>;
    using ATraits = std::allocator_traits<Alloc>;
    using PTraits = typename ATraits::template rebind_traits<T*>;
    using PAlloc = typename PTraits::allocator_type;

public:
    explicit use_custom_buffers(Alloc alloc) : m_alloc(std::move(alloc)) {}

    use_custom_buffers *set_buffers(size_t count, size_t size) {
        PAlloc pointer_alloc(m_alloc);
        T **buffers = PTraits::allocate(pointer_alloc, count);
        for (size_t i = 0; i < count; ++i) {
            buffers[i] = ATraits::allocate(m_alloc, size);
        }
        this->Base::set_buffers(count, buffers, size);
        return this;
    }

private:
    Alloc m_alloc;
};

There's still work to do. count and size are strange names, especially since size is also a count (of T objects), not the "size" of any entity in the program. I decided to punt on the question of what to call Base::set_buffers. The arguments to set_buffers are in a weird order (length, pointer, other-length). It is unclear from your description whether anyone in the codebase actually cares about use_custom_buffers_base or whether it could be completely hidden away in a detail namespace — or, indeed, inlined into use_custom_buffers, since the inheritance relationship here seems like it's doing more harm than good.

---

Finally, a possible performance issue: Why are you calling allocate in a loop?

        for (size_t i = 0; i < count; ++i) {
            buffers[i] = ATraits::allocate(m_alloc, size);
        }

Surely it would be more performant to allocate just once and then use pointers into different parts of that buffer?

        auto ptr = ATraits::allocate(m_alloc, count * size);
        for (size_t i = 0; i < count; ++i) {
            buffers[i] = ptr + (i * size);
        }