diff --git a/rclcpp/include/rclcpp/any_service_callback.hpp b/rclcpp/include/rclcpp/any_service_callback.hpp index 5d6f3ee7b7..93903e473e 100644 --- a/rclcpp/include/rclcpp/any_service_callback.hpp +++ b/rclcpp/include/rclcpp/any_service_callback.hpp @@ -33,21 +33,27 @@ namespace rclcpp namespace detail { -template -struct can_be_nullptr : std::false_type {}; - -// Some lambdas define a comparison with nullptr, -// but we see a warning that they can never be null when using it. -// We also test if `T &` can be assigned to `nullptr` to avoid the issue. -template -#ifdef __QNXNTO__ -struct can_be_nullptr() == nullptr)>>: std::true_type {}; -#else -struct can_be_nullptr() == nullptr), decltype(std::declval() = nullptr)>> - : std::true_type {}; -#endif +// Returns true if the callback stored in the given variant is empty/null. +// The stored alternatives are std::function objects, whose operator bool is +// false only when empty; lambdas and bind expressions are never empty, so this +// flags only genuine null targets (e.g. a null function pointer). Checking the +// stored std::function avoids probing the raw callable type with `== nullptr` / +// `= nullptr`, which is ill-formed for some callables and broke compilation of +// create_service() with certain callbacks/compilers (see ros2/rclcpp#1742). +template +bool +callback_is_null(const VariantT & callback) +{ + return std::visit( + [](const auto & cb) { + if constexpr (std::is_same_v, std::monostate>) { + return true; + } else { + return !cb; + } + }, + callback); +} } // namespace detail // Forward declare @@ -62,9 +68,7 @@ class AnyServiceCallback : callback_(std::monostate{}) {} - template< - typename CallbackT, - typename std::enable_if_t::value, int> = 0> + template void set(CallbackT && callback) { @@ -102,51 +106,11 @@ class AnyServiceCallback // of all the above workaround ... callback_ = std::forward(callback); } - } - template< - typename CallbackT, - typename std::enable_if_t::value, int> = 0> - void - set(CallbackT && callback) - { - if (!callback) { + // Reject a null target (e.g. a null function pointer or empty std::function). + if (detail::callback_is_null(callback_)) { throw std::invalid_argument("AnyServiceCallback::set(): callback cannot be nullptr"); } - // Workaround Windows issue with std::bind - if constexpr ( - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrWithRequestHeaderCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallbackWithServiceHandle - >::value) - { - callback_.template emplace(callback); - } else { - // the else clause is not needed, but anyways we should only be doing this instead - // of all the above workaround ... - callback_ = std::forward(callback); - } } // template> diff --git a/rclcpp/include/rclcpp/generic_service.hpp b/rclcpp/include/rclcpp/generic_service.hpp index 8a4468aa1f..1cf32a33bf 100644 --- a/rclcpp/include/rclcpp/generic_service.hpp +++ b/rclcpp/include/rclcpp/generic_service.hpp @@ -23,6 +23,7 @@ #include #include +#include "rclcpp/any_service_callback.hpp" #include "rclcpp/typesupport_helpers.hpp" #include "rosidl_runtime_c/service_type_support_struct.h" @@ -45,9 +46,7 @@ class GenericServiceCallback : callback_(std::monostate{}) {} - template< - typename CallbackT, - typename std::enable_if_t::value, int> = 0> + template void set(CallbackT && callback) { @@ -85,50 +84,10 @@ class GenericServiceCallback // of all the above workaround ... callback_ = std::forward(callback); } - } - template< - typename CallbackT, - typename std::enable_if_t::value, int> = 0> - void - set(CallbackT && callback) - { - if (!callback) { - throw std::invalid_argument("AnyServiceCallback::set(): callback cannot be nullptr"); - } - // Workaround Windows issue with std::bind - if constexpr ( - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrWithRequestHeaderCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallback - >::value) - { - callback_.template emplace(callback); - } else if constexpr ( // NOLINT - rclcpp::function_traits::same_arguments< - CallbackT, - SharedPtrDeferResponseCallbackWithServiceHandle - >::value) - { - callback_.template emplace(callback); - } else { - // the else clause is not needed, but anyways we should only be doing this instead - // of all the above workaround ... - callback_ = std::forward(callback); + // Reject a null target (e.g. a null function pointer or empty std::function). + if (detail::callback_is_null(callback_)) { + throw std::invalid_argument("GenericServiceCallback::set(): callback cannot be nullptr"); } } diff --git a/rclcpp/test/rclcpp/test_any_service_callback.cpp b/rclcpp/test/rclcpp/test_any_service_callback.cpp index ef46155d99..767a919e40 100644 --- a/rclcpp/test/rclcpp/test_any_service_callback.cpp +++ b/rclcpp/test/rclcpp/test_any_service_callback.cpp @@ -109,3 +109,46 @@ TEST_F(TestAnyServiceCallback, set_and_dispatch_defered_with_service_handle) { EXPECT_EQ(nullptr, any_service_callback_.dispatch(nullptr, request_header_, request_))); EXPECT_EQ(callback_with_header_calls, 1); } + +namespace +{ +void free_service_callback( + const std::shared_ptr, + std::shared_ptr) +{} +} // namespace + +// Regression for ros2/rclcpp#1742: setting the callback with a plain function +// pointer or std::function must compile and work. The previous can_be_nullptr +// SFINAE trait was ill-formed for some callables and broke create_service(). +TEST_F(TestAnyServiceCallback, set_and_dispatch_function_pointer) { + void (* callback)( + const std::shared_ptr, + std::shared_ptr) = &free_service_callback; + EXPECT_NO_THROW(any_service_callback_.set(callback)); + EXPECT_NO_THROW( + EXPECT_NE(nullptr, any_service_callback_.dispatch(nullptr, request_header_, request_))); +} + +TEST_F(TestAnyServiceCallback, set_and_dispatch_std_function) { + std::function, + std::shared_ptr)> callback = free_service_callback; + EXPECT_NO_THROW(any_service_callback_.set(callback)); + EXPECT_NO_THROW( + EXPECT_NE(nullptr, any_service_callback_.dispatch(nullptr, request_header_, request_))); +} + +TEST_F(TestAnyServiceCallback, set_null_function_pointer_throws) { + void (* callback)( + const std::shared_ptr, + std::shared_ptr) = nullptr; + EXPECT_THROW(any_service_callback_.set(callback), std::invalid_argument); +} + +TEST_F(TestAnyServiceCallback, set_null_std_function_throws) { + std::function, + std::shared_ptr)> callback = nullptr; + EXPECT_THROW(any_service_callback_.set(callback), std::invalid_argument); +}