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.

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

More and safer node interfaces' getters #1035

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ template<
rclcpp::node_interfaces::NodeBaseInterface *
get_node_base_interface_from_pointer(NodeType node_pointer)
{
if (!node_pointer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

throw std::invalid_argument("node pointer cannot be nullptr");
}
return node_pointer->get_node_base_interface().get();
}

Expand All @@ -98,6 +101,9 @@ template<
rclcpp::node_interfaces::NodeBaseInterface *
get_node_base_interface_from_pointer(NodeType node_pointer)
{
if (!node_pointer) {
throw std::invalid_argument("node pointer cannot be nullptr");
}
return node_pointer->get_node_base_interface();
}

Expand All @@ -115,6 +121,37 @@ get_node_base_interface_from_pointer(NodeType node_shared_pointer)
return get_node_base_interface_from_pointer(node_shared_pointer->get());
}

// If NodeType has a method called get_node_base_interface() which returns a shared pointer.
template<
typename NodeType,
typename std::enable_if<has_get_node_base_interface<
typename std::remove_pointer<NodeType>::type,
std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>
>::value, int>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>
get_shared_node_base_interface_from_pointer(NodeType node_pointer)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
if (!node_pointer) {
throw std::invalid_argument("node pointer cannot be nullptr");
}
return node_pointer->get_node_base_interface();
}

// Forward shared_ptr's to const node pointer signatures.
template<
typename NodeType,
typename std::enable_if<std::is_same<
NodeType,
typename std::shared_ptr<typename std::remove_pointer<NodeType>::type::element_type> *
>::value, int>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>
get_shared_node_base_interface_from_pointer(NodeType node_shared_pointer)
{
return get_shared_node_base_interface_from_pointer(node_shared_pointer->get());
}

} // namespace detail

/// Get the NodeBaseInterface as a pointer from a pointer to a "Node like" object.
Expand All @@ -125,7 +162,7 @@ template<
rclcpp::node_interfaces::NodeBaseInterface *
get_node_base_interface(NodeType node_pointer)
{
// Forward pointers to detail implmentation directly.
// Forward pointers to detail implementation directly.
return detail::get_node_base_interface_from_pointer(node_pointer);
}

Expand All @@ -139,10 +176,36 @@ template<
rclcpp::node_interfaces::NodeBaseInterface *
get_node_base_interface(NodeType && node_reference)
{
// Forward references to detail implmentation as a pointer.
// Forward references to detail implementation as a pointer.
return detail::get_node_base_interface_from_pointer(&node_reference);
}

/// Get the NodeBaseInterface as an std::shared_ptr from a pointer to a "Node like" object.
template<
typename NodeType,
typename std::enable_if<std::is_pointer<NodeType>::value, int>::type = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@hidmic hidmic Mar 30, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

>
std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>
get_shared_node_base_interface(NodeType node_pointer)
{
// Forward pointers to detail implementation directly.
return detail::get_shared_node_base_interface_from_pointer(node_pointer);
}

/// Get the NodeBaseInterface as an std::shared_ptr from a "Node like" object.
template<
typename NodeType,
typename std::enable_if<
!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>
get_shared_node_base_interface(NodeType && node_reference)
{
// Forward references to detail implementation as a pointer.
return detail::get_shared_node_base_interface_from_pointer(&node_reference);
}

} // namespace node_interfaces
} // namespace rclcpp

Expand Down
212 changes: 212 additions & 0 deletions rclcpp/include/rclcpp/node_interfaces/get_node_clock_interface.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,212 @@
// Copyright 2020 Open Source Robotics Foundation, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#ifndef RCLCPP__NODE_INTERFACES__GET_NODE_CLOCK_INTERFACE_HPP_
#define RCLCPP__NODE_INTERFACES__GET_NODE_CLOCK_INTERFACE_HPP_

#include <memory>
#include <utility>
#include <type_traits>

#include "rclcpp/node_interfaces/node_clock_interface.hpp"

/// This header provides the get_node_clock_interface() template function.
/**
* This function is useful for getting the NodeClockInterface pointer from
* various kinds of Node-like classes.
*
* It's able to get the NodeClockInterface pointer so long as the class
* has a method called ``get_node_clock_interface()`` which returns
* either a pointer (const or not) to a NodeClockInterface or a
* std::shared_ptr to a NodeClockInterface.
*/

namespace rclcpp
{
namespace node_interfaces
{

namespace detail
{

// This is a meta-programming checker for if a given Node-like object has a
// getter called get_node_clock_interface() which returns various types,
// e.g. const pointer or a shared pointer.
template<typename NodeType, typename ReturnType>
struct has_get_node_clock_interface
{
private:
template<typename T>
static constexpr
auto
check(T *)->typename std::is_same<
decltype(std::declval<T>().get_node_clock_interface()),
ReturnType
>::type;

template<typename>
static constexpr
std::false_type
check(...);

public:
using type = decltype(check<NodeType>(nullptr));
static constexpr bool value = type::value;
};

// If NodeType is a pointer to NodeClockInterface already (just normal function overload).
inline
rclcpp::node_interfaces::NodeClockInterface *
get_node_clock_interface_from_pointer(rclcpp::node_interfaces::NodeClockInterface * pointer)
{
return pointer;
}

// If NodeType has a method called get_node_clock_interface() which returns a shared pointer.
template<
typename NodeType,
typename std::enable_if<has_get_node_clock_interface<
typename std::remove_pointer<NodeType>::type,
std::shared_ptr<rclcpp::node_interfaces::NodeClockInterface>
>::value, int>::type = 0
>
rclcpp::node_interfaces::NodeClockInterface *
get_node_clock_interface_from_pointer(NodeType node_pointer)
{
if (!node_pointer) {
throw std::invalid_argument("node pointer cannot be nullptr");
}
return node_pointer->get_node_clock_interface().get();
}

// If NodeType has a method called get_node_clock_interface() which returns a pointer.
template<
typename NodeType,
typename std::enable_if<has_get_node_clock_interface<
typename std::remove_pointer<NodeType>::type,
rclcpp::node_interfaces::NodeClockInterface *
>::value, int>::type = 0
>
rclcpp::node_interfaces::NodeClockInterface *
get_node_clock_interface_from_pointer(NodeType node_pointer)
{
if (!node_pointer) {
throw std::invalid_argument("node pointer cannot be nullptr");
}
return node_pointer->get_node_clock_interface();
}

// Forward shared_ptr's to const node pointer signatures.
template<
typename NodeType,
typename std::enable_if<std::is_same<
NodeType,
typename std::shared_ptr<typename std::remove_pointer<NodeType>::type::element_type> *
>::value, int>::type = 0
>
rclcpp::node_interfaces::NodeClockInterface *
get_node_clock_interface_from_pointer(NodeType node_shared_pointer)
{
return get_node_clock_interface_from_pointer(node_shared_pointer->get());
}

// If NodeType has a method called get_node_clock_interface() which returns a shared pointer.
template<
typename NodeType,
typename std::enable_if<has_get_node_clock_interface<
typename std::remove_pointer<NodeType>::type,
std::shared_ptr<rclcpp::node_interfaces::NodeClockInterface>
>::value, int>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeClockInterface>
get_shared_node_clock_interface_from_pointer(NodeType node_pointer)
{
if (!node_pointer) {
throw std::invalid_argument("node pointer cannot be nullptr");
}
return node_pointer->get_node_clock_interface();
}

// Forward shared_ptr's to const node pointer signatures.
template<
typename NodeType,
typename std::enable_if<std::is_same<
NodeType,
typename std::shared_ptr<typename std::remove_pointer<NodeType>::type::element_type> *
>::value, int>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeClockInterface>
get_shared_node_clock_interface_from_pointer(NodeType node_shared_pointer)
{
return get_shared_node_clock_interface_from_pointer(node_shared_pointer->get());
}

} // namespace detail

/// Get the NodeClockInterface as a pointer from a pointer to a "Node like" object.
template<
typename NodeType,
typename std::enable_if<std::is_pointer<NodeType>::value, int>::type = 0
>
rclcpp::node_interfaces::NodeClockInterface *
get_node_clock_interface(NodeType node_pointer)
{
// Forward pointers to detail implementation directly.
return detail::get_node_clock_interface_from_pointer(node_pointer);
}

/// Get the NodeClockInterface as a pointer from a "Node like" object.
template<
typename NodeType,
typename std::enable_if<
!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
>::type = 0
>
rclcpp::node_interfaces::NodeClockInterface *
get_node_clock_interface(NodeType && node_reference)
{
// Forward references to detail implementation as a pointer.
return detail::get_node_clock_interface_from_pointer(&node_reference);
}

/// Get the NodeClockInterface as an std::shared_ptr from a pointer to a "Node like" object.
template<
typename NodeType,
typename std::enable_if<std::is_pointer<NodeType>::value, int>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeClockInterface>
get_shared_node_clock_interface(NodeType node_pointer)
{
// Forward pointers to detail implementation directly.
return detail::get_shared_node_clock_interface_from_pointer(node_pointer);
}

/// Get the NodeClockInterface as an std::shared_ptr from a "Node like" object.
template<
typename NodeType,
typename std::enable_if<
!std::is_pointer<typename std::remove_reference<NodeType>::type>::value, int
>::type = 0
>
std::shared_ptr<rclcpp::node_interfaces::NodeClockInterface>
get_shared_node_clock_interface(NodeType && node_reference)
{
// Forward references to detail implementation as a pointer.
return detail::get_shared_node_clock_interface_from_pointer(&node_reference);
}

} // namespace node_interfaces
} // namespace rclcpp

#endif // RCLCPP__NODE_INTERFACES__GET_NODE_CLOCK_INTERFACE_HPP_
Loading