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.

Sign up for GitHub

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

Jump to bottom

Added support for const member functions #763

Merged
merged 4 commits into from
Jun 14, 2019
Merged

Added support for const member functions #763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cba4ffc
Added support for const member functions
esteve Jun 14, 2019
70aa49f
Added signature for Windows
esteve Jun 14, 2019
392165f
Fix style
esteve Jun 14, 2019
6a429b6
Attempt at fixing function_traits for macOS
esteve Jun 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions rclcpp/include/rclcpp/function_traits.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,24 @@ struct function_traits<
: function_traits<ReturnTypeT(Args ...)>
{};

// std::bind for object const methods
template<typename ClassT, typename ReturnTypeT, typename ... Args, typename ... FArgs>
#if defined _LIBCPP_VERSION // libc++ (Clang)
struct function_traits<std::__bind<ReturnTypeT (ClassT::*)(Args ...) const, FArgs ...>>
#elif defined _GLIBCXX_RELEASE // glibc++ (GNU C++ >= 7.1)
struct function_traits<std::_Bind<ReturnTypeT(ClassT::*(FArgs ...))(Args ...) const>>
Copy link
Contributor

@allenh1 allenh1 Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve Not questioning any of this, but could you explain to me what exactly this is? I can tell it's the result of a std::bind (which makes this compiler dependent), but I'm not following the two variadic templates here at all. If you have time to explain, I'd love to learn why this patch is the fix.

Copy link
Member Author

@esteve esteve Jun 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allenh1 sure, no problem. Let's say we have this struct:

struct A {
    int member_const(float x, double y) const {
        (void)x;
        (void)y;
        return 1;
    }

    int member_non_const(float x, double y) {
        (void)x;
        (void)y;
        return 2;
    }
};

They are the same, but one of them is declared as const. So because of the slightly different signature, the template specializations we have for std::bind don't match. Its real signature for glibc++ and GCC >= 7.1 is the following:

std::_Bind<int (A::*(A*, std::_Placeholder<1>, std::_Placeholder<2>))(float, double) const>

For which we don't have any match in function_traits, the fix consists in duplicating the signatures we have for object members and just add a const where needed. However, I don't have a macOS or Windows machine at hand, so I haven't updated the signatures for them. To fix it, we just need to run the function_traits testsuite, and update the specializations to match the signatures.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve Ah, that makes a lot of sense. Thank you very much!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve I ran it on my windows machine for you: https://gist.github.com/allenh1/0b879d6a28c831615c68f307934a5e0f

Not building at the moment, it seems. But this should have all you need in it! Let me know if you need anything else.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allenh1 awesome, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@allenh1 I justed updated the specialization with the info you pasted, could you try it again? Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve I think it's fixed. I have a failure, but my build is not up to date yet (and it is unrelated to function_traits). I'll build my workspace and let you know if it fails.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@esteve Definitely working on my machine

#elif defined __GLIBCXX__ // glibc++ (GNU C++)
struct function_traits<std::_Bind<std::_Mem_fn<ReturnTypeT (ClassT::*)(Args ...) const>(FArgs ...)>>
#elif defined _MSC_VER // MS Visual Studio
struct function_traits<
std::_Binder<std::_Unforced, ReturnTypeT(__cdecl ClassT::*)(Args ...) const, FArgs ...>
>
#else
#error "Unsupported C++ compiler / standard library"
#endif
: function_traits<ReturnTypeT(Args ...)>
{};

// std::bind for free functions
template<typename ReturnTypeT, typename ... Args, typename ... FArgs>
#if defined _LIBCPP_VERSION // libc++ (Clang)
Expand Down
24 changes: 24 additions & 0 deletions rclcpp/test/test_function_traits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ struct ObjectMember
return 7;
}

int callback_one_bool_const(bool a) const
{
(void)a;
return 7;
}

int callback_two_bools(bool a, bool b)
{
(void)a;
Expand Down Expand Up @@ -394,6 +400,16 @@ TEST(TestFunctionTraits, argument_types) {
rclcpp::function_traits::function_traits<decltype(bind_one_bool)>::template argument_type<0>
>::value, "Functor accepts a bool as first argument");

auto bind_one_bool_const = std::bind(
&ObjectMember::callback_one_bool_const, &object_member, std::placeholders::_1);

static_assert(
std::is_same<
bool,
rclcpp::function_traits::function_traits<decltype(bind_one_bool_const)>::template
argument_type<0>
>::value, "Functor accepts a bool as first argument");

auto bind_two_bools = std::bind(
&ObjectMember::callback_two_bools, &object_member, std::placeholders::_1,
std::placeholders::_2);
Expand Down Expand Up @@ -561,6 +577,14 @@ TEST(TestFunctionTraits, check_arguments) {
static_assert(
rclcpp::function_traits::check_arguments<decltype(bind_one_bool), bool>::value,
"Functor accepts a single bool as arguments");

auto bind_one_bool_const = std::bind(
&ObjectMember::callback_one_bool_const, &object_member, std::placeholders::_1);

// Test std::bind functions
static_assert(
rclcpp::function_traits::check_arguments<decltype(bind_one_bool_const), bool>::value,
"Functor accepts a single bool as arguments");
}

/*
Expand Down