More and safer node interfaces' getters

[hidmic]

This pull requests indirectly addresses ros2/geometry2#244 by adding more and safer node interfaces' getters, and then using them where applicable (e.g. in rclcpp_action::create_*(), indirectly solving the aforementioned issue).

I'm opening this as a draft PR for @Karsten1987 to weigh in and for @wjwwood to comment on how this may or may not be in line with the ongoing rclcpp API review.

[Karsten1987]

This looks okay to me.

The problem I see though is that there is a lot of code duplicated. I've actually taken a short of this quite some time ago and still feel that this code might actually be better generated:
e6dd86d

[Karsten1987]

I would enhance this with rcpputils pointer traits to validate that it's actually a pointer:
https://github.com/ros2/rcpputils/blob/master/include/rcpputils/pointer_traits.hpp#L73

[Karsten1987]

std::is_pointer does not work with smart pointers. I recommend using https://github.com/ros2/rcpputils/blob/master/include/rcpputils/pointer_traits.hpp#L73

[hidmic]

IIUC that's what overloads in line 203 and line 150 are for. Though I agree with you it's somewhat odd.

We can for sure do as you suggest too.

[ivanpauno]

rcpputils::is_pointer wouldn't be correct.

It accepts an unique_ptr, and then:

base_interface = get_shared_node_base_interface(unique_node_ptr);

isn't possible and it won't compile.

base_interface = get_shared_node_base_interface(std::move(unique_node_ptr));

would be allowed, but probably not what we want.

Note: It's not currently possible to create a Node unique_ptr (we use std::shared_from_this), so the last example isn't currently possible either.

---

In general, code accepting any pointer like object as a raw pointer is not recommended.
See https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#S-resource.

Avoiding to write shared_node.get() or *shared_node might seem like more comfortable, but it doesn't express intention correctly (ownership isn't clear).

[hidmic]

It accepts an unique_ptr

Fair point. It won't compile either way, but the message does get more obscure.

it doesn't express intention correctly (ownership isn't clear).

Fair point as well. I can only guess it is how it is just for the sake of making it easier on the user.

[hidmic]

The problem I see though is that there is a lot of code duplicated.

I absolutely agree. And I'd be fine with generating code too, as you suggest with e6dd86d, though we'd be duplicating no less, only avoiding the maintenance burden. But to solve that we'd have to move away from the (ever increasing?) list of interface getter methods in rclcpp::Node and friends, and that's out of scope.

[Karsten1987]

My concern here was really only about the maintenance burden. If we re-iterate over this code - and I am sure we will at some point - it makes things way easier as we only touch code once.
I am not saying we should move away from the getter methods.

[hidmic]

@wjwwood any thoughts on this? If not, I'll just go ahead with @Karsten1987's e6dd86d commit and turn this into a real PR.

[ivanpauno]

IMO, we should only have this, and the others should be deprecated in favor of:

get_shared_node_base_interface_from_pointer(node_pointer).get();

though I don't mind strongly.

[hidmic]

What if shared ownership isn't available? I can't tell what use cases the original author had in mind to make all these getters return a non-owning reference.

[ivanpauno]

What if shared ownership isn't available?

It's currently not the case.
I don't see a reason why allowing to some Node like objects to return a shared pointer of the interface, and allowing to others just to return a raw pointer.

[ivanpauno]

rcpputils::is_pointer wouldn't be correct.

It accepts an unique_ptr, and then:

base_interface = get_shared_node_base_interface(unique_node_ptr);

isn't possible and it won't compile.

base_interface = get_shared_node_base_interface(std::move(unique_node_ptr));

would be allowed, but probably not what we want.

Note: It's not currently possible to create a Node unique_ptr (we use std::shared_from_this), so the last example isn't currently possible either.

---

In general, code accepting any pointer like object as a raw pointer is not recommended.
See https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#S-resource.

Avoiding to write shared_node.get() or *shared_node might seem like more comfortable, but it doesn't express intention correctly (ownership isn't clear).

[ivanpauno]

Using empy to generate this code sounds like a great idea!

[hidmic]

#1069 has superseded this PR. Closing.