type trait: rclcpp::is_pointer<T>

[Karsten1987]

This is a type trait to check whether a parameter is a pointer. This is essentially an extension to std::is_pointer<T> which also checks for std::shared_ptr<T> as well as std::unique_ptr<T>.

The use case for this is to pass in a pointer to a node independently whether it's a shared pointer or a raw pointer. Motivation for this see here: https://github.com/ros/diagnostics/pull/112/files#r310801219

Signed-off-by: Karsten Knese karsten@openrobotics.org

[ivanpauno]

The implementation LGTM.
This could live in rcpputils repo.

If you want to use it here: https://github.com/ros/diagnostics/blob/4edd6dfe6af8950d63d29d9f436bef87a015c316/diagnostic_updater/include/diagnostic_updater/diagnostic_updater.hpp#L373-L380, I don't agree with the usage. The Updater constructor should take a rclcpp::Node &.

[Karsten1987]

I am okay with having it on rcpputils, I can reopen the PR over there.

The motivation for this is actually exactly to avoid forcing a constructor taking a ‘rclcpp::Node’ because it doesn’t allow for its derivates or similar nodes such as ‘rclcpp::LifecycleNode’.

[ivanpauno]

The motivation for this is actually exactly to avoid forcing a constructor taking a ‘rclcpp::Node’ because it doesn’t allow for its derivates or similar nodes such as ‘rclcpp::LifecycleNode’.

That sounds good. But I don't know why accepting all the possible pointers to them.
unique_ptr shouldn't be allowed, as both Node and LifecycleNode inherit from enable_shared_from_this.
Taking a shared_ptr is not idiomatic, as the method isn't retaining part ownership after the call (see https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#S-resource R32 to R36).

We are already doing something similar of what you want (which it doesn't follow the guidelines I copied either). See:

• https://github.com/ros2/rclcpp/blob/dc3c36c7f05c06f504041a2ab2c8df1160438607/rclcpp/include/rclcpp/node_interfaces/get_node_base_interface.hpp
• https://github.com/ros2/rclcpp/blob/dc3c36c7f05c06f504041a2ab2c8df1160438607/rclcpp/include/rclcpp/node_interfaces/get_node_timers_interface.hpp
• https://github.com/ros2/rclcpp/blob/dc3c36c7f05c06f504041a2ab2c8df1160438607/rclcpp/include/rclcpp/node_interfaces/get_node_topics_interface.hpp

[Karsten1987]

I totally agree, however there is no free functions for logger nor for parameters as far as I know which makes it hard for me to use it.

[thomas-moulard]

Adding some doc here would be great IMHO

[thomas-moulard]

+1 for rcpputils, happy to review the PR there.

[ivanpauno]

I totally agree, however there is no free functions for logger nor for parameters as far as I know which makes it hard for me to use it.

I think we should add get_*_interface for those. The other problem with this functions, is that they are returning a raw pointer, when they should return the shared_ptr (which is available too).
@Karsten1987 If you agree with that, I can implement it and open a PR.

I don't like the idea of templated functions accepting any kind of pointer, so I'm not in favor of adding rclcpp::is_pointer.

[emersonknapp]

we can't hide these because templates, but I assume that the helpers aren't part of the public API. might it make sense to nest them in a rcpputils::impl namespace so at least it's clear they're not part of the public API?

And agreed with thomas on value of adding some doc strings

[emersonknapp]

@Karsten1987 friendly ping, is the plan to move this to rcpputils?

[Karsten1987]

closing in favor of ros2/rcpputils#19