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 <#3 (comment)>, 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 == 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.