Skip to content

Commit

Permalink
refactor AnySubscriptionCallback and add/deprecate callback signatures (
Browse files Browse the repository at this point in the history
ros2#1598)

* refactor AnySubscriptionCallback to...

use std::variant and make the dispatch functions constexpr,
avoiding runtime dispatching.

Also, deprecate the std::function<void (std::shared_ptr<MessageT>)> signature,
as it is unsafe to share one shared_ptr when multiple subscriptions take it,
because they could mutate MessageT while others are using it. So you'd have to
make a copy for each subscription, which is no different than the
std::unique_ptr<MessageT> signature or the user making their own copy in a
shared_ptr from the const MessageT & signature or the
std::shared_ptr<const MessageT> signature.

Added a std::function<void (const std::shared_ptr<const MessageT> &)> signature
to go along side the existing
std::function<void (std::shared_ptr<const MessageT>)> signature.

Removed redundant 'const' before pass-by-value signatures, e.g.
std::function<void (const shared_ptr<const MessageT>)> became
std::function<void (shared_ptr<const MessageT>)>.
This will not affect API or any users using the old style.

Signed-off-by: William Woodall <[email protected]>

* fix use of std::bind, free functions, etc. using new function_traits::as_std_function<>

Signed-off-by: William Woodall <[email protected]>

* fix use of const MessageT & callbacks by fixing subscriptin_traits

Signed-off-by: William Woodall <[email protected]>

* fix deprecation warnings

Signed-off-by: William Woodall <[email protected]>

* use target_compile_features to require c++17 for downstream users of rclcpp

Signed-off-by: William Woodall <[email protected]>

* uncrustify

Signed-off-by: William Woodall <[email protected]>

* cpplint

Signed-off-by: William Woodall <[email protected]>

* use target_compile_features(... cxx_std_17)

Signed-off-by: William Woodall <[email protected]>

* Keep both std::shared_ptr<const MessageT> and const std::shared_ptr<const MessageT> & signatures.

The const std::shared_ptr<const MessageT> & signature is being kept because it
can be more flexible and efficient than std::shared_ptr<const MessageT>, but
costs realtively little to support.

The std::shared_ptr<const MessageT> signature is being kept because we want to
avoid deprecating it and causing disruption, and because it is convenient to
write, and in most cases will not materially impact the performance.

Signed-off-by: William Woodall <[email protected]>

* defer deprecation of the shared_ptr<MessageT> sub callbacks

Signed-off-by: William Woodall <[email protected]>

* fix unused variable warning

Signed-off-by: William Woodall <[email protected]>

* small fixups to AnySubscriptionCallback

Signed-off-by: William Woodall <[email protected]>

* add check for unset AnySubscriptionCallback in dispatch methods

Signed-off-by: William Woodall <[email protected]>

* update dispatch methods to handle all scenarios

Signed-off-by: William Woodall <[email protected]>

* updated tests for AnySubscriptionCallback, include full parameterized i/o matrix

Signed-off-by: William Woodall <[email protected]>

* fixup test with changed assumption

Signed-off-by: William Woodall <[email protected]>

* remove use of std::unary_function, which was removed in c++17

Signed-off-by: William Woodall <[email protected]>

* silence c++17 warnings on windows for now

Signed-off-by: William Woodall <[email protected]>
  • Loading branch information
wjwwood authored Apr 3, 2021
1 parent 95adde2 commit 1037822
Show file tree
Hide file tree
Showing 35 changed files with 884 additions and 429 deletions.
13 changes: 9 additions & 4 deletions rclcpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ find_package(rosidl_typesupport_cpp REQUIRED)
find_package(statistics_msgs REQUIRED)
find_package(tracetools REQUIRED)

# Default to C++14
# TODO(wjwwood): remove this when gtest can build on its own, when using target_compile_features()
# Default to C++17
if(NOT CMAKE_CXX_STANDARD)
set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD 17)
endif()
if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
# About -Wno-sign-conversion: With Clang, -Wconversion implies -Wsign-conversion. There are a number of
Expand Down Expand Up @@ -174,8 +175,12 @@ foreach(interface_file ${interface_files})
include/rclcpp/node_interfaces/get_${interface_name}.hpp)
endforeach()

add_library(${PROJECT_NAME}
${${PROJECT_NAME}_SRCS})
add_library(${PROJECT_NAME} ${${PROJECT_NAME}_SRCS})
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17)
# TODO(wjwwood): address all deprecation warnings and then remove this
if(WIN32)
target_compile_definitions(${PROJECT_NAME} PUBLIC "_SILENCE_ALL_CXX17_DEPRECATION_WARNINGS")
endif()
target_include_directories(${PROJECT_NAME} PUBLIC
"$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>"
"$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/include>"
Expand Down
Loading

0 comments on commit 1037822

Please sign in to comment.