Function copy construction fails based on Function& or const Function&

[daniel-brosche]

Tested on Linux/GCC 6.3.0...

[rigtorp]

Hmm, yeah the implementation is partly broken

[daniel-brosche]

This was needed to achieve a correct copy construction -> something like that:

 struct Data {
        Function<void()> on_constructor;
        Function<void()> on_destructor;

        Data(const Function<void()>& on_constructor, const Function<void()>& on_destructor) {
            this->on_constructor = on_constructor;
            this->on_destructor = on_destructor;

            this->on_constructor();
        }

        ~Data() { this->on_destructor(); }
    };

 void test() {
    bool on_constructor_called = false;
    bool on_destructor_called = false;

    Function<void()> on_constructor = [&]() { on_constructor_called = true; };
    Function<void()> on_destructor = [&]() { on_destructor_called = true; };

    Data* d = new Data(on_constructor, on_destructor);
    delete d;
 }

[rigtorp]

Right now the copy constructor just copies the raw memory, that only works if std::is_trivally_copyable_v<Func> == true.

On Tue, Sep 28, 2021 at 1:37 PM Daniel B. @.***> wrote:

I have adapted some small parts regarding copy construction https://github.com/daniel-brosche/Function/tree/bugfix/copy_construction_fails

This was needed to achieve something like that:

struct Data { Function<void()> on_constructor; Function<void()> on_destructor;

    Data(const Function<void()>& on_constructor, const Function<void()>& on_destructor) {
        this->on_constructor = on_constructor;
        this->on_destructor = on_destructor;

        this->on_constructor();
    }

    ~Data() { this->on_destructor(); }
};

void test() { bool on_constructor_called = false; bool on_destructor_called = false;

Function<void()> on_constructor = [&]() { on_constructor_called = true; };
Function<void()> on_destructor = [&]() { on_destructor_called = true; };

Data* d = new Data(on_constructor, on_destructor);
delete d;

}

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rigtorp/Function/pull/3#issuecomment-929106600, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABLO24RB7L4VMZGMH6GJC3UEGSG5ANCNFSM5E45OONA .

[daniel-brosche]

Right now the copy constructor just copies the raw memory, that only works if std::is_trivally_copyable_v<Func> == true.

Do you mean it would be better to check it like that in the copy constructor?

[rigtorp]

Well I have partially implemented copy support for non-trivial Functors: https://github.com/rigtorp/Function/blob/master/Function.h#L123

This manager function needs to be used to copy non-trivial functions.

[daniel-brosche]

Without my modifications the gcc prints:

cannot convert 'Function<void()>::Storage {aka std::aligned_storage<1008ul, 8ul>::type}' to 'void*' in argument passing

when I use the current version of copy constructor

Function(const Function& other) {
        if (other) {
            other.manager(data, other.data, Operation::Clone);
            invoker = other.invoker;
            manager = other.manager;
        }
    }

Using address operator does not finally solve the issue because other.data is const.

Function(const Function& other) {
        if (other) {
            other.manager(&data, &other.data, Operation::Clone);
            invoker = other.invoker;
            manager = other.manager;
        }
    }

Therefore gcc does also complain with an error about converting const other to void*

invalid conversion from 'const void*' to 'void*' [-fpermissive]

Only in combination using the complile-flag -fpermissive helps to implicitly convert these types which is not a good practice. In the meantime I have found a cleaner option instead of using const_cast by adapting the manager function (see last commit).

The other modfication regarding:

Function(Function& other) { other.swap(*this); }

is needed because if a construction is based on Function& then the following constructor will be called:

Function(F&& f) {
        using f_type = typename std::decay<F>::type;
        static_assert(alignof(f_type) <= alignof(Storage), "invalid alignment");
        static_assert(sizeof(f_type) <= sizeof(Storage), "storage too small");
       ...
}

and gcc prints the following error:

static assertion failed: storage too small

Therefore I did add a further specialized constructor to handle this error.

 Function(Function& other) : Function(const_cast<const Function&>(other)) {}

This is in my opinion the cleaneast solution (see last commit).

[rigtorp]

I guess manager function also needs a Move operation to implement move assignment.

Have you checked how GCC implements std::function? I haven't been using this code that I wrote as an experiment many years ago.