Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
gpalmer-latai opened this issue Jul 23, 2024 · 5 comments
Open

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

gpalmer-latai opened this issue Jul 23, 2024 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@gpalmer-latai
Copy link
Contributor

gpalmer-latai commented Jul 23, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

gpalmer-latai commented Jul 24, 2024

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 elBoberido added bug Something isn't working good first issue Good for newcomers labels Jul 24, 2024
@elBoberido
Copy link
Member

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
Copy link
Contributor Author

gpalmer-latai commented Jul 25, 2024

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
Copy link
Member

So a great good first issue :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants