eclipse-iceoryx / iceoryx

Eclipse iceoryx™ - true zero-copy inter-process-communication
https://iceoryx.io
Apache License 2.0
1.66k stars 387 forks source link

iox::function claims to support moving functions, but doesn't #2319

Open gpalmer-latai opened 2 months ago

gpalmer-latai commented 2 months ago

Required information

In the docstring for the iox::function interface, it is claimed that

///        In contrast to iox::function_ref iox::function objects own everything needed
///        to invoke the underlying callable and can be safely stored.
///        They also support copy and move semantics in natural way
///        by copying or moving the underlying callable.

However in the actual implementation docstring this is contradicted:

Furthermore, the storable_functor stores a copy
    // which avoids implicit misbehaviors or ownership problems caused by implicit conversion.

And indeed if you attempt to wrap a lambda with a move capture, such as

  auto unique_int = std::make_unique<int>(42);
  auto do_stuff = [unique_int = std::move(unique_int)]{++(*unique_int);};
  iox::function<void(void)> do_stuff_wrapper(std::move(do_stuff));
  do_stuff_wrapper();

You'll get a compile error:

external/iceoryx/iceoryx_hoofs/functional/include/iox/detail/storable_function.inl:207:22: error: call to implicitly-deleted copy constructor of 'StoredType' (aka '(lambda at <redacted>main.cc:29:19)')
    new (m_callable) StoredType(functor);
                     ^          ~~~~~~~
external/iceoryx/iceoryx_hoofs/functional/include/iox/detail/storable_function.inl:32:5: note: in instantiation of function template specialization 'iox::storable_function<128, void ()>::storeFunctor<(lambda at <redacted>/main.cc:29:19), void>' requested here
    storeFunctor(functor);
    ^
<redacted>/main.cc:30:29: note: in instantiation of function template specialization 'iox::storable_function<128, void ()>::storable_function<(lambda at <redacted>/main.cc:29:19), void>' requested here
  iox::function<void(void)> do_stuff_wrapper(std::move(do_stuff));
                            ^
<redacted>/main.cc:29:20: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
  auto do_stuff = [unique_int = std::move(unique_int)]{++(*unique_int);};
                   ^
<redacted>/usr/include/c++/11.2.0/bits/unique_ptr.h:468:7: note: 'unique_ptr' has been explicitly marked deleted here
      unique_ptr(const unique_ptr&) = delete;
      ^
In file included from <redacted>/main.cc:8:
In file included from <redacted>:
In file included from <redacted>:
In file included from external/iceoryx/iceoryx_posh/include/iceoryx_posh/internal/mepoo/segment_manager.hpp:21:
In file included from external/iceoryx/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_config.hpp:19:
In file included from external/iceoryx/iceoryx_posh/include/iceoryx_posh/mepoo/segment_config.hpp:21:
In file included from external/iceoryx/iceoryx_posh/include/iceoryx_posh/mepoo/mepoo_config.hpp:19:
In file included from external/iceoryx/iceoryx_posh/include/iceoryx_posh/iceoryx_posh_types.hpp:24:
In file included from external/iceoryx/iceoryx_hoofs/functional/include/iox/function.hpp:20:
In file included from external/iceoryx/iceoryx_hoofs/functional/include/iox/detail/storable_function.hpp:208:
external/iceoryx/iceoryx_hoofs/functional/include/iox/detail/storable_function.inl:229:27: error: call to implicitly-deleted copy constructor of '(lambda at <redacted>/main.cc:29:19)'
    new (dest.m_callable) CallableType(*obj);
                          ^            ~~~~
external/iceoryx/iceoryx_hoofs/functional/include/iox/detail/storable_function.inl:210:34: note: in instantiation of function template specialization 'iox::storable_function<128, void ()>::copy<(lambda at <redacted>/main.cc:29:19)>' requested here
    m_operations.copyFunction = &copy<StoredType>;
                                 ^
external/iceoryx/iceoryx_hoofs/functional/include/iox/detail/storable_function.inl:32:5: note: in instantiation of function template specialization 'iox::storable_function<128, void ()>::storeFunctor<(lambda at <redacted>/main.cc:29:19), void>' requested here
    storeFunctor(functor);
    ^
<redacted>/main.cc:30:29: note: in instantiation of function template specialization 'iox::storable_function<128, void ()>::storable_function<(lambda at <redacted>/main.cc:29:19), void>' requested here
  iox::function<void(void)> do_stuff_wrapper(std::move(do_stuff));
                            ^
<redacted>/main.cc:29:20: note: copy constructor of '' is implicitly deleted because field '' has a deleted copy constructor
  auto do_stuff = [unique_int = std::move(unique_int)]{++(*unique_int);};
                   ^
<redacted>/usr/include/c++/11.2.0/bits/unique_ptr.h:468:7: note: 'unique_ptr' has been explicitly marked deleted here
      unique_ptr(const unique_ptr&) = delete;

Operating system: E.g. Ubuntu 20.04 LTS

Compiler version: E.g. GCC 9.4.0

Eclipse iceoryx version: On an older checkout of main, but should apply to main as well.

Observed result or behaviour: iox::function does not support move-only functors

Expected result or behaviour: It should support move-only functors, as documented in the API.

Conditions where it occurred / Performed steps: Try to wrap a lambda with a move-capture.

Additional helpful information

elBoberido commented 2 months ago

@gpalmer-latai how big of an issue is this right now? We are currently quite busy with the iceoryx2 C and C++ bindings and I would look into this once we are done with them.

gpalmer-latai commented 2 months ago

It's not a super huge priority. There are workarounds like using a shared_ptr or just using std::function or something instead. These solutions aren't pretty though and we'd still like to use a "fixed size" function wrapper.

I mostly wanted to make sure to document the issue in the off chance someone did have a few free cycles or came across the same issue.

elBoberido commented 2 months ago

I marked it as good first issue ... let's hope it's not a bottomless pit once someone has a closer look at it

gpalmer-latai commented 2 months ago

Haha yeah. I mean, on the one hand maybe overloading that function with a signature allowing rvalue references is enough (at least - it appears std::function does this: https://en.cppreference.com/w/cpp/utility/functional/function/function).

On the other hand, I've seen several C++ weekly videos by Jason Turner where he basically explains implementing std::function correctly is notoriously hard and a great exercise to learn about a lot of advanced C++ features.

elBoberido commented 2 months ago

So a great good first issue :)