Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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 initial 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) // fix
    {
        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; }
}

Update: above code will not be modified in order to keep it in sync with the answers give. Improvements will be applied here.

share|improve this question
    
Why reinvent working technology that is part of the standard already? –  Loki Astari Jul 29 at 23:39
    
@LokiAstari This class is a little bit different than std::function in its allocation model. It even states that in the first sentence. std::function has allocator support but it doesn't do anything according to a DR I remember reading. –  Rapptz Jul 29 at 23:42
    
@LokiAstari I could be wrong, but from my experiments it seems that std::function + custom allocator cannot do stack-based allocation like this. –  StackedCrooked Jul 29 at 23:46
    
@LokiAstari I'm trying out different ways to enable fast task queueing when doing task-based concurrency. (Still considering various solutions at this point.) –  StackedCrooked Jul 30 at 0:34
    
OK I read that the wrong way around. Please don't use the term Heap/Stack storage its confusing and not particularly accurate description for C++. In this situation you are not guaranteeing heap storage hear you are just guaranteeing that the storage is local to the object. So more accurately you want automatic memory (scoped locally to the current object) rather than dynamic memory (scoped dynamically). –  Loki Astari Jul 30 at 0:34

2 Answers 2

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.

share|improve this answer
1  
Why not just alignas(std::intptr_t)? –  glampert Jul 30 at 2:48
    
@glampert Yeah, silly of me. Updated. –  Yuushi Jul 30 at 3:13

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.