Custom fixed-space block memory allocator

Question

This is a fixed-size memory allocator.
The core memory block is an unsigned char pointer which gets allocated on initialization. Other than that, it has pretty basic allocation and block safety checking functionality.

Would appreciate any feedback.

template <size_t block_size, size_t blocks>
class MemoryAllocator
{
 // internal memory type
 using uchar = unsigned char;
 // byte patterns to mark memory blocks
 enum Pattern : uchar
 {
 UNALLOCATED = 0xAA,
 ALLOCATED = 0xBB
 };
 // meta data
 static constexpr size_t pad_bytes = 3;
 static constexpr size_t hb_size = pad_bytes + block_size;
 static constexpr size_t bytes_allocated = hb_size * blocks;
 // internal memory block
 uchar* data_;
 size_t open_blocks_;
 
 public:
 
 /**
 * @brief Construct a new Memory Allocator object.
 * 
 */
 MemoryAllocator() : open_blocks_(blocks)
 {
 data_ = new uchar[bytes_allocated];
 std::memset(data_, 0, bytes_allocated);
 // set header blocks to free
 for (size_t i = 0; i < blocks; ++i)
 std::memset(data_ + (i * hb_size), Pattern::UNALLOCATED, pad_bytes);
 }
 /**
 * @brief Destructor
 * 
 */
 ~MemoryAllocator() noexcept
 {
 delete [] data_;
 }
 /**
 * @brief Allocates memory.
 * 
 * @return void* 
 */
 template <typename T, typename... Args>
 T* Allocate(Args&&... args) noexcept
 {
 if constexpr (!std::is_same<T, void>::value)
 static_assert((alignof(T) + sizeof(T)) <= block_size, "Block cannot hold T.");
 if (open_blocks_ == 0)
 return nullptr;
 for (size_t i = 0; i < blocks; ++i)
 {
 size_t offset = i * hb_size;
 // safety check
 bool valid = true;
 for (size_t j = 0; j < pad_bytes; ++j)
 {
 if (data_[offset + j] != Pattern::UNALLOCATED)
 {
 valid = false;
 break;
 }
 }
 if (!valid) continue;
 std::memset(data_ + offset, Pattern::ALLOCATED, pad_bytes);
 --open_blocks_;
 if constexpr (std::is_same<T, void>::value)
 {
 return (data_ + offset + pad_bytes);
 }
 else
 {
 return new(data_ + offset + pad_bytes) T(args...);
 }
 }
 return nullptr;
 }
 /**
 * @brief Frees memory.
 * 
 * @param block 
 */
 void Free(void* block)
 {
 uchar* mem = reinterpret_cast<uchar*>(block) - pad_bytes;
 // safety check
 for (size_t i = 0; i < pad_bytes; ++i)
 if (mem[i] != Pattern::ALLOCATED)
 throw std::runtime_error("Freeing corrupted block!");
 std::memset(mem, Pattern::UNALLOCATED, pad_bytes);
 ++open_blocks_;
 if (open_blocks_ > blocks)
 throw std::runtime_error("Too many blocks freed");
 }
 /**
 * @brief Whether or not there is room for more allocations.
 * 
 * @return true 
 * @return false 
 */
 bool CanAllocate() const noexcept
 {
 return open_blocks_ > 0;
 }
 
 // prevent copying of any kind
 MemoryAllocator& operator=(MemoryAllocator& rhs) = delete;
 MemoryAllocator(const MemoryAllocator& rhs) = delete;
 MemoryAllocator(MemoryAllocator&& rhs) = delete;
};
/**
 * @brief Works with the MemoryAllocator to allocate for classes.
 * 
 * @tparam T 
 * @tparam blocks 
 */
template <typename T, size_t blocks>
struct ClassAllocator : public MemoryAllocator<sizeof(T) + alignof(T), blocks>
{
 template <typename... Args>
 void* Allocate(Args&&... args) noexcept
 {
 return MemoryAllocator<sizeof(T) + alignof(T), blocks>::template Allocate<void>(std::forward<Args>(args)...);
 }
};

Answer 1

The name is misleading, as the class doesn't conform to the standard C++ Allocator concept.

Missing headers:

#include <cstring>
#include <cstddef>
#include <stdexcept>
#include <type_traits>
#include <utility>

Misspelt std::size_t as size_t throughout.

We can make the data_ pointer const:

 uchar *const data_;

if we initialise it in the constructor:

 MemoryAllocator()
 : data_(new uchar[bytes_allocated]),
 open_blocks_(blocks)

Consider using a std::unique_ptr for data_, so that we can just default the destructor (rule of zero).

enum Pattern doesn't have any value as a type, so just use a couple of ordinary constants. And change their names away from all-caps, so they don't look like scary macros:

// byte patterns to mark memory blocks
static constexpr uchar pattern_free = 0xAA;
static constexpr uchar pattern_busy = 0xBB;

I'd consider encapsulating the "all bytes equal value" test into a function:

private:
 static bool all_eq(uchar const* start, uchar const* end, uchar val)
 {
 for (auto p = start; p != end; ++p) {
 if (*p != val) { return false; }
 }
 return true;
 }

Or simply (and more clearly)

private:
 static bool all_eq(uchar const* start, uchar const* end, uchar val)
 {
 return std::all_of(start, end, [val](uchar c){ return c==val; });
 }

Then we get

 // safety check
 if (!all_eq(data_ + offset, data + offset + pad_bytes, pattern_free)) continue;

and

 // safety check
 if (!all_eq(m, mem + pad_bytes, pattern_busy)) {
 throw std::runtime_error("Freeing corrupted block!");
 }

It's probably worth naming data_ + offset, as it's used several times.

Answer 2

In addition to Toby Speight's answer:

Use std::fill_n instead of memcpy()

The former is more C++, and more type-safe. So instead of:

std::memset(data_, 0, bytes_allocated);

You can write:

std::fill_n(data_, bytes_allocated, 0);

Avoid giving new names to standard types

Instead of declaring uchar as an alias to unsigned char, just use the latter. Yes, it's a bit more to type, but now you don't have to remember that uchar is an unsigned char. If you really want to avoid a bit of typing, consider using a standard type like std::uint8_t, or perhaps even better:

Consider using std::byte

Since you are using C++17 features, you can also use std::byte * as the type for data_. You need to make a few changes to avoid having to explicitly cast things to/from std::byte:

1. Instead of using an enum Pattern, declare these constants as static constexpr variables, as Toby Speight already recommended, like so:

   static constexpr std::byte UNALLOCATED{0xAA};
   static constexpr std::byte ALLOCATED{0xBB};
   

2. Use std::fill_n instead of std::memset():

   std::fill_n(data_, bytes_allocated, UNALLOCATED);
   

   Note that if you still want to fill a region with zeroes, you need to write:

   std::fill_n(data_, bytes_allocated, std::byte{});
   

Use lower case for member function names

It is common in C++ to use lower case (or rather, snake_case) for functions and variables, and use PascalCase for types.

Have ClassAllocator::Allocate() return a T *

Since ClassAllocator knows the type of the objects you want to allocate, it would be nice if it would return a pointer of the correct type, instead of a void *, so the caller doesn't have to manually cast it.