Simple C++ object pool

Question

I am studying computer science and just for the sake of practise I thought of implementing an object pool.

This is my first implementation, it is simple and works correctly, but I would love any thoughts on it. Currently, the memory is allocated on the stack, but I am thinking of using policy based design to allow heap allocation as well.

Here is the code of the pool:

#include <iostream>
#include <memory>
#include <functional>

/**
 * Generic class for object pools.
 */
template <class T, int SZ, class Initialiser, class Releaser> class StackObjectPool {
public:
    using pointer = std::unique_ptr<T,std::function<void(T*)> >;
    using PoolType = StackObjectPool<T,SZ,Initialiser, Releaser>;

    StackObjectPool() {}
    StackObjectPool(const PoolType& orig) = delete;
    ~StackObjectPool() {}

    /**
     * Acquires an object not being currently used
     * @return pointer to the acquired object
     * @throw std::out_of_range if all the objects inside the pool are being used
     */
    pointer acquire() {
        unsigned int index = 0; // look for the first free object
        while(m_occupied_registry[index]) ++index;
        if(index >= SZ) throw std::out_of_range("Pool exceeded its size");
        m_occupied_registry[index] = true; // mark it as currently in use
        m_initialiser(&m_objects[index]); // initialise it

        //return an unique_ptr that calls release when reset
        return pointer(&m_objects[index], [this](T* element)->void{release(element);});
    }

    friend std::ostream& operator<<(std::ostream& ostream, const PoolType& pool) {
        for(unsigned int index = 0; index < SZ; ++index) {
            std::cout<<(pool.m_occupied_registry[index]?"[occupied] ":"[free]\n");
            if(pool.m_occupied_registry[index]) {
                std::cout<<pool.m_objects[index]<<"\n";
            }
        }
        return ostream;
    }
private:

    void release(T* element) {
        unsigned int index = 0;
        while(&m_objects[index] != element) ++index;
        m_occupied_registry[index] = false; // mark the released element as free
        m_releaser(element); // call release functor
    }

    Initialiser m_initialiser;
    Releaser m_releaser;
    bool m_occupied_registry[SZ] {0};
    T m_objects[SZ];
};

And here is a Test class and a simple main which I have used to test the pool:

class Test {
public:
    Test():m_a(0),m_b('0'){
        std::cout<<"Construct default Test"<<*this<<"\n";
    }
    Test(int a, char b): m_a(a), m_b(b) { 
        std::cout<<"Construct Test"<<*this<<"\n";
    }
    Test(const Test& orig): m_a(orig.m_a),m_b(orig.m_b) {
        std::cout<<"Construct cpy Test"<<*this<<"\n";
    }
    ~Test() {
        std::cout<<"Delete Test"<<*this<<"\n";
    }
    friend std::ostream& operator<<(std::ostream& ostream, const Test& test) {
        ostream<<"["<<test.m_a<<",'"<<test.m_b<<"']";
        return ostream;
    }
private:
    int m_a;
    char m_b;
};

struct Initialiser {
    void operator() (Test* element) {
        std::cout<<"Initialising Test"<<*element<<"\n";
    }
};

struct Releaser {
    void operator() (Test* element) {
        std::cout<<"Releasing Test"<<*element<<"\n";
    }
};

int main(int argc, char** argv) {
    using TestPool = StackObjectPool<Test,5, Initialiser, Releaser>;
    TestPool pool;
    TestPool::pointer pt1, pt2,pt3,pt4,pt5;

    std::cout<<pool;

    pt1= pool.acquire();
    std::cout<<"Aquired pt:"<<*pt1<<"\n";
    //pt1.reset();

    std::cout<<pool;

    pt2 = pool.acquire();
    std::cout<<"Aquired pt:"<<*pt2<<"\n";
    pt2.reset();

    std::cout<<pool;

    pt3 = pool.acquire();
    std::cout<<"Aquired pt:"<<*pt3<<"\n";
    //pt3.reset();

    std::cout<<pool;
    return 0;
}

Answer 1

the implementation itself already feels comfortable. It might not say much, though, I find it important that code uses a single style. Congrats!

That said, every code has a few remarks:

• Why would you name your template argument SZ? Wouldn't MaxSize be a better name?
• Would it not be more interesting to make the type of SZ unsigned as a negative size will be incorrect
• Should this class be final?
• You already use =delete, why don't you use =default for the Ctor and Dtor?
• I would use a subclass to represent the deletor of the unique_ptr as std::function has a performance overhead
• In the streaming operator, why do you use std::cout instead of ostream
• Try to explain what your code should do, not the c++ concepts you are using

It's also nice to have a test on this code, though, it feels a bit lacking:

• Why don't you try to acquire more than 5 elements?
• The Initializer and Releaser don't represent real-case usages

So in conclusion: This code ain't bad

Answer 2

You might change:

void release(T* element, unsigned int index) {
    if (&m_objects[index] != element) {
            std::cerr << "no match index and element\n";
            return; // or exit(1);
    }
    m_occupied_registry[index] = false; // mark the released element as free
    m_releaser(element); // call release functor
}

and at the acquire change the return line:

    return pointer(&m_objects[index], [this,index](T* element)->void{release(element,index);});

The above will improve the efficiency of the release operation, as you don't need to search for the element.