Function wrapper like std::function that uses "small buffer" allocation

Question

My Function class is similar to std::function with the small-buffer optimization. However, it always uses a small buffer and static asserts if the required size exceeds it.

I would like a review for:

• correct use of the low-level features
• general class design and interoperability with standard library

Here is my code:

#include <cassert>
#include <functional>
#include <iostream>
#include <new>
#include <utility>
#include <array>


template<typename Signature>
struct Function;

template<typename R, typename ...Args>
struct Function<R(Args...)>
{
    Function() : storage()
    {}

    template<typename F>
    Function(const F& f)
    {
        static_assert(alignof(F) <= alignof(Function), "");
        static_assert(sizeof(f) <= sizeof(storage), "");

        new (storage.data()) Impl<F>(f);
    }

    Function(const Function& rhs) : 
        storage(rhs.storage)
    {
        if (rhs.valid())
        {
            rhs.getImpl().clone(data());
        }
    }

    Function(Function&& rhs) noexcept :
        storage(rhs.storage)
    {
        rhs.storage = Storage();
    }

    Function& operator=(Function rhs) noexcept
    {
        std::swap(storage, rhs.storage); // not sure if safe
        return *this;
    }

    ~Function()
    {
        if (valid())
        {
            getImpl().~Base();
        }
    }

    R operator()(Args&& ...args) const
    {
        if (!valid())
        {
            throw std::bad_function_call();
        }
        return getImpl().call(std::forward<Args>(args)...);
    }

private:
    struct Base
    {
        virtual ~Base() {}
        virtual R call(Args&& ...args) const = 0;
        virtual void clone(void* where) const = 0;
    };


    template<typename F>
    struct Impl : Base
    {
        Impl(const F& f) : f(f) {}

        R call(Args&& ...args) const override final
        { return f(std::forward<Args>(args)...); }

        void clone(void* where) const override final
        { new (where) Impl<F>(*this); }

        F f;
    };

    // convenience methods
    bool valid() const
    { return storage != Storage(); }

    const void* data() const
    { return static_cast<const void*>(storage.data()); }

    void* data()
    { return static_cast<void*>(storage.data()); }

    const Base& getImpl() const
    { assert(valid()); return *static_cast<const Base*>(data()); }

    Base& getImpl()
    { assert(valid()); return *static_cast<Base*>(data()); }

    typedef std::array<long, 4> Storage; // long is probably max-align
    Storage storage;
};


int main()
{
    Function<int(int)> increment = [](int n) {
        return n + 1;
    };
    Function<int(int)> decrement = [](int n) {
        return n - 1;
    };


    std::cout << increment(3) << std::endl;    
    std::cout << decrement(3) << std::endl;

    increment = std::move(decrement);
    std::cout << increment(3) << std::endl;


    // calling the moved-from decrement
    try
    {
        decrement(3);
        assert(false);
    }
    catch (std::bad_function_call& e)
    {
        std::cout << e.what() << std::endl;
    }
}

Answer 1

Generally, I can't see any real problems, but there are a few things that I'm going to point out:

In the assignment operator, you use std::swap(storage, rhs.storage) and declare it as noexcept. This should probably be conditionally noexcept, based on whether the swap implementation for the given array type (in this case, array<long>) is noexcept. That being said, I'd be really, really surprised if this wasn't always true.

The only other issues I can see are to do with alignment and storage size, which you've basically dealt with thanks to the two static_asserts in the constructor for Function (although they should both have a string detailing exactly what happened if either of them fails). As you're probably aware, there are precious few guarantees about function pointer sizes (especially when you start dealing with a combination of member functions, virtual functions, and multiple inheritance). 4 * sizeof(long) should be enough space to hold anything like this, but this is implementation defined; there are no guarantees.

With regards to alignment, I'm less confident. long is only guaranteed by the standard to be at least 4 bytes, and this is true even for 64-bit platforms. Since storage is actually holding the function itself, the alignment should be made more explicit, and shouldn't rely on the (potentially less stringent) alignment of long. I'd personally suggest:

alignas(std::intptr_t) Storage storage;

which (as far as I can see) should ensure storage will be aligned on the correct boundary.

Answer 2

My two problems:

You have a magic object you use to test for validity. I know a zero'ed array of longs is unlikely to be equivalent to a valid F object. But it just sticks in my throat a bit to use a magic object. Also if it ever goes wrong then finding that error is going to be really hard.

You are using the equivalent to a mem-copy on an F type object in certain situations. To be type safe you must always use the constructor on creation and the assignment must correctly call the assignment of the the underlying F object. That will also make destruction simpler as you don't need to test validity (and thus don't need a magic object).

---

First the move constructor you are doing the equivalent of a memory copy of the storage without taking into account the type of what is inside. But you are still applying the destructor to these objects.

 Function<A,B>    x(anF);          // Fine (placement new).
 Function<A,B>    y(std::move(x)); // Mem-Copy the storage.

Here y has mem-copied the F part over from x (So no constructor was called). You probably want to move it.

Also the assignment operator does not work.
Apologies to @Rapptz (now that I have dug into this I understand the complexities more). I still don't believe the code was correct, BUT @Rapptz did spot the depth of the problem long before I did and was working towards a solution.

  Function<A,B>   z(anotherF);
  z = y;                           // Again we are doing Mem-Copy
                                   // of the storage area you need to
                                   // use the assignment operator.

So I think you need to do this:

Function(Function&& rhs) noexcept
{
    // Pass the work to the Impl<F> object.

    getImpl().move(std::move(rhs.getImp()));
                                    // Note we need another virtual
}                                   // function here that does the move
                                    // on placement new (close to clone)

Function& operator=(Function rhs) noexcept
{
    // Pass the work to the Impl<F> object.

    getImpl().copy(rhs.getImpl());  // Note we need another virtual
    return *this;                   // fucntion here that does the copy.
}                                   // very close to the clone.

Now because we have always used the valid move constructor to move the F object held in storage (and there is no default constructor). We don't need to check the object in storage is valid on destruction (just call the destructor).

~Function()
{
    getImpl().~Base();
}

So Impl<F> needs:

    // A bit of a guess.
    // Not compiled (took me a lot of guessing to get this the
    // way I actually want.
    void copy(Base& rhs) const override final
    {
        f = static_cast<Impl<F>>(rhs).f;
    }
    void move(Base&& rhs) const override final
    {
        new (&f) F(std::move(static_cast<Impl<F>>(rhs).f));
    }

If you want to do "move assignment" you will need to add another virtual in Impl<F> to cover the move assignment case.

Answer 3

Here is my code review, I see 2 things:

• Here we can read:

std::max_align_t is usually synonymous with the largest scalar type, which is long double on most platforms, and its alignment requirement is either 8 or 16.

So the alignment of storage is wrong I think, it should probably be made explicit:

alignas (std::max_align_t) Storage storage;

• Also, you ask in your question about interoperability with standard library, so I assume you want to reproduce the same API, if this is the case, some public methods and operators are missing, as you can see here : swap and operator bool.