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?

• Kerrek SB: Is this the entire code? Where is the base declaration of uses_custom_buffers? And is it an option to follow the STL allocator design?
• Xeo: @Kerrek: The base is a specialization of itself.
• Kerrek SB: @Xeo: Does that work? I think I misread one of my compiler errors! D'oh. Thanks!
• 0xbadf00d: @Kerrek SB - I'm following the STL allocator design. The template argument "alloc" could be any STL conform allocator type. That's exactly the same technique the STL containers are using.
• Kerrek SB: I see. Hm. My compiler suggests you say typename alloc::template rebind<int_type*>::other pointer_alloc;

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);
 }

• Toby Speight: Writing size_t instead of std::size_t means one of: (a) using C headers instead of C++ headers; (b) non-portable assumptions about the contents of headers; or (c) use of using namespace std. I don't recommend any of those, so consequently I disagree with you on that recommendation. Rest of the review is great: +1.
• Quuxplusone: @TobySpeight: FWIW, I have nothing against (a); I agree with your recommendation against (c). Can you elaborate on (b)? That is, let's assume that my assumption is specifically "size_t is provided by <cstddef>." You claim this assumption is "non-portable." I counter-claim that this assumption is portable (that is, code using this assumption will compile on every C++ platform in the world; which is the best anyone can hope for from "portability"). Can you name a platform where such code wouldn't compile? Lastly, there's at least (d): using std::size_t;.
• Toby Speight: The problem with (b) is that although it might work with the compilers you know of, the Standard does not mandate that it will work with all compilers (including the really good compilers we'll want to use in The Future). I prefer to rely as much as possible on what's documented (and therefore a reportable bug when I'm wrong) than on what's optional. To some extent, it's a version of the principle that made the Internet Protocol suite so successful - be conservative in what you emit and liberal in what you accept.
• Toby Speight: And yes, (d) is better than (a) to (c).