Reusable storage for array of objects

Question

My goal is to have a memory pool non-template class that is used to store arrays of objects. The same memory pool object must be reusable for a different array (difference size, different type and/or alignment).

I already posted a series of questions on SO about this subject:

• Correctly using type-punning and erasure for array of objects
• type-punning and strict aliasing rule for array of objects
• type-punning: omitting placement new and destructors
• type-punning with std::aligned_alloc for array of objects
• What exactly does the standard array form placement new expression?

I tried to sum-up all the discussions in these questions into a C++14 code. The "interesting" part is the Buffer class:

#include <cstddef>
#include <functional>
#include <iostream>

// Object constructible from a double
// forcing alignment
struct alignas(16) SFloat {
    float val = 0.f;
    SFloat() { std::cout << "Constructing a SFloat with default value\n"; };
    SFloat(const double v) : val(static_cast<float>(v)) {
        std::cout << "Constructing a SFloat from " << v << '\n';
    };
    SFloat& operator=(SFloat&& f) {
        val = f.val;
        std::cout << "Move-assigning from a SFloat " << f.val << '\n';
        return *this;
    }
    ~SFloat() { std::cout << "Destructing a SFloat holding " << val << '\n'; }
};
// Serialization of Float objects
std::ostream& operator<<(std::ostream& o, SFloat const& f) {
    return o << f.val;
}

// just for the sake of the example: p points to at least a sequence of 3 T
// probably not the best implem, but compiles without conversion warning with
// SFloat and float.
template <typename T>
void load(T* p) {
    std::cout << "starting load\n";
    p[0] = static_cast<T>(3.14);
    p[1] = static_cast<T>(31.4);
    p[2] = static_cast<T>(314.);
    std::cout << "ending load\n";
}

// type-punning reusable buffer
// holds a non-typed buffer (actually a char*) that can be used to store any
// types, according to user needs
struct Buffer {
    // destructing functor storage
    // required to call the correct object destructors
    // using std::function to store a copy of the lambda used
    // @1 is there a way to avoid std::function?
    std::function<void(Buffer*)> Destructors = [](Buffer*) {};
    // buffer address
    unsigned char* p = nullptr;
    // aligned buffer address
    unsigned char* paligned = nullptr;
    // number of stored elements
    size_t n = 0;
    // buffer size in bytes
    size_t s = 0;

    // computes the smallest positive offset in bytes that must be applied to p
    // in order to have alignment a a is supposed to be a valid alignment
    std::size_t OffsetForAlignement(unsigned char const* const ptr,
                                    std::size_t a) {
        std::size_t res = reinterpret_cast<std::size_t>(ptr) % a;
        if (res) {
            return a - res;
        } else {
            return 0;
        }
    }
    // allocates a char buffer large enough for N object of type T and
    // default-construct them
    // N must be > 0
    template <typename T>
    T* DefaultAllocate(const std::size_t N) {
        // Destroy previously stored objects, supposedly ends lifetime of the
        // array object that contains them
        Destructors(this);
        std::size_t RequiredSize = sizeof(T) * N + alignof(T);
        if (s < RequiredSize) {
            std::cout << "Requiring " << RequiredSize << " bytes of storage\n";
            // @2 creating a storage of RequiredSize bytes
            // what would be the C++17+ way of do that? std::aligned_alloc?
            p = reinterpret_cast<unsigned char*>(std::realloc(p, RequiredSize));
            s = RequiredSize;
            // here should do something for the case where p is nullptr
            paligned = p + OffsetForAlignement(p, alignof(T));
        }
        // @3 Form1 placement array new default construction: ill-defined in
        // C++14?
        // expecting starting an array object lifetime and the lifetime of
        // contained objects
        // expecting pointer arithmetic to be valid on tmp T*
        // T *tmp = new (p) T[N];
        // @4 Form2 individually creating packed object in storage
        // expecting starting an array object lifetime and the lifetime of
        // contained objects
        // expecting pointer arithmetic to be valid on tmp T*
        unsigned char* pos = paligned;
        T* tmp = reinterpret_cast<T*>(paligned);
        for (std::size_t i = 0; i < N; ++i) {
            new (pos) T();
            pos += sizeof(T);
        }

        // update nb of objects
        n = N;
        // create destructors functor
        // @5 supposedly ends the lifetime of the array object and of the
        // contained objects
        Destructors = [](Buffer* pobj) {
            T* ToDestruct = reinterpret_cast<T*>(pobj->p);
            // Delete elements in reverse order of creation
            while (pobj->n > 0) {
                --(pobj->n);
                // should be std::Destroy(ToDestruct[n]) in C++17
                // I should provide my own implementation in C++14 in order to
                // distinguish between fundamental types and other ones
                // @ how to formally en the lifetime of a fundamental objects?
                // merely rewrite on its memory location?
                ToDestruct[(pobj->n)].~T();
            }
            // @6 How to formally end the array object lifetime?
        };
        return tmp;
    }
    // deallocate objects in buffer but not the buffer itself
    // actually useless
    // template <typename T>
    // void Deallocate() {
    //     Destructors(this);
    // }
    ~Buffer() {
        // Ending objects lifetime
        Destructors(this);
        // Releasing storage
        std::free(p);
    }
};

int main() {
    constexpr std::size_t N0 = 7;
    constexpr std::size_t N1 = 3;
    Buffer B;
    std::cout << "Test on SomeClass\n";
    SFloat* ps = B.DefaultAllocate<SFloat>(N0);
    ps[0] = 3.14;
    *(ps + 1) = 31.4;
    ps[2] = 314.;
    std::cout << ps[0] << '\n';
    std::cout << ps[1] << '\n';
    std::cout << *(ps + 2) << '\n';
    std::cout << "Test on float\n";
    // reallocating, possibly using existing storage, for a different type and
    // size
    float* pf = B.DefaultAllocate<float>(N1);
    pf[0] = 3.14f;
    *(pf + 1) = 31.4f;
    pf[2] = 314.f;
    std::cout << pf[0] << '\n';
    std::cout << pf[1] << '\n';
    std::cout << *(pf + 2) << '\n';
    return 0;
}

Live demo I would appreciate a review on this Buffer class, in C++14. Yet I'm also interested in upgrade to C++17 and beyond.
I've placed // @# numbered comment in the code for focus on specific implementation issues/questions.

N.B. my concern in SO was that these type-punning techniques, though well defined for single objects, seemed to be ill-defined when speaking of arrays.

Note: I previously had a std::function which I replaced by a simple pointer to function. Yet I don't understand why the pointer does not become dangling when leaving the DefaultAllocate function.

Answer 1

Use the return value of placement-new

In your code you write:

T* tmp = reinterpret_cast<T*>(paligned);

However, at this point there is no live object of type T at paligned. You might create one later, but depending on the version of C++ you are using and how much of a language lawyer you are, use of tmp after this might still be undefined behavior. There are various ways you can get a valid pointer to T, for example using std::launder() or C++23's std::start_lifetime_as(), but there is a much simpler way: use the return value of the placement-new operator. Note that you can combine this with array allocation, so:

T* tmp = new(pos) T[N];

You might ask: why does this matter? Apart from some esoteric machines where pointers work differently depending on the type of object they point to, static analysis tools might also complain if your code is not correct according to the C++ abstract machine's memory model.

Once you have a valid tmp you can reinterpret_cast<> it safely to and from char*, unsigned char* and std::byte*.

Possible incorrect alignment after reallocating

If you call DefaultAllocate() a second time on a given Buffer, it could be that the new T fits in the old space, but has a larger alignment requirement than the old T. Thus, this could result in returning an incorrectly aligned pointer.

You have to recalculate paligned even if s >= RequiredSize.

Incorrect pointer used when destructing objects

You use pobj->p in Destructors when it should have been pobj->paligned at the very least. But also take into account what I said above, and ensure you use a pointer that is derived from the return value of the placement-new expressions.

Note that you can capture a T* in the lambda expression, so there is no need to pass it a Buffer* and to use type-erased pointers.

Missing exception handling

Unless the constructor of T is marked noexcept, you must consider the possibility that construction of any object of type T might cause an exception to be thrown. If so, you must make sure that you destroy any objects you have constructed so far.

C++17 has some functions to help with this, like for example std::uninitialized_value_construct(). If you cannot use it yet in a C++14 code base, it might still be helpful to look at how it is implemented.

Destructors should never throw an exception, so you can mark the destructor of Buffer noexcept.

Use std::uintptr_t instead of std::size_t when aligning

There are computer architectures where pointers are larger than std::size_t. The correct type to use to convert a pointer to an unsigned integer type is std::uintptr_t. On the other hand, this type is an optional part of the standard, but I haven't heard of any architecture where this wasn't present.

Use private to avoid exposing implementation details

Users of Buffer should not be able to change any of the member variables, as this could cause problems later. Use private and public to only expose those parts of the interface that are meant for the users of it.

Naming

Some variables are named with capitals, others only with lower case. The same goes for function names. Pick one style and stick with it. I recommend you use either snake_case or camelCase, as those seem to be the two most used styles. But do capitalize the names of types.

Also avoid unnecessarily short names, like p, n and s. These could be renamed to storage, count and size for example, although n is so commonly used for "number of" that it would be OK to keep it like that, similar to how i is a commonly used name for a loop counter.

Is it useful?

A Buffer is a very limited memory pool: you can only have one active allocation at a time. You might sometimes avoid reallocating memory, but new and delete are not that expensive. So your main() function could be replaced with:

int main() {
    …
    std::cout << "Test on SomeClass\n";
    SFloat* ps = new SFloat[N0];
    …
    // reallocating, possibly using existing storage, for a different type and size
    delete[] ps;
    float* pf = new float[N1];
    …
}

The comment is still valid: delete followed by new could very well reuse the same memory. Of course, raw new and delete should be avoided, std::vector is the appropriate thing to use here.

Answer 2

First impressions

The code compiles cleanly with a picky set of warnings enabled, and runs clean under Valgrind's memcheck. That's always a good start.

I believe SFloat and load() are part of the test/example, rather than of Buffer - I would have found it easier to read if the production code and its exercise were more clearly separated.

We're missing #include <cstdlib> for the declarations of std::realloc()and std::free(). It's not portable to rely on an implementation transitively including this via some other standard library header.

std::size_t is assumed in the global namespace in a couple of places, which is (at least theoretically) non-portable:

// number of stored elements
size_t n = 0;
// buffer size in bytes
size_t s = 0;

Allocation can fail

This code is a problem:

        p = reinterpret_cast<unsigned char*>(std::realloc(p, RequiredSize));

If std::realloc() returns a null pointer, then we have lost our pointer to the memory block. We need to retain this pointer until we that the reallocation was successful:

            Destructors = [](Buffer*) {};
            auto realloced = reinterpret_cast<unsigned char*>(std::realloc(p, RequiredSize));
            if (!realloced) { throw std::bad_alloc{}; }
            p = realloced;

(If you're on Linux, use ulimit -v to exercise this case in testing.)

Constructors can throw

When we construct our array of T, we violate our class's invariant for n if one of them throws:

        for (std::size_t i = 0; i < N; ++i) {
            new (pos) T();
            pos += sizeof(T);
        }

Because n still holds its old value at this point, the destructor will clean up the wrong number of elements.

From C++17 onwards, we could replace this loop with std::uninitialized_default_construct_n, which deals with such exceptions sensibly (it's an "all or nothing" operation). And we can use std::destroy_n to replace the loop in Destructors.

The Destructors function variable

The reason this exists is to remember the type of object that's stored. It's only ever invoked on this, so it might be easier to read if we just capture this - or perhaps just capture the start and length:

    std::function<void()> Destructors = []{};

        Destructors = [tmp,N]{ std::destroy_n(tmp, N); };

Spelling

OffsetForAlignement misspells "alignment".