From 48c168a5ca2e39c83d84a08ddafda86dd8d4d7b8 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 5 Nov 2018 14:21:07 -0800 Subject: [PATCH] [rcl] Remove redundant error checking and set explicit error message with '_is_valid' functions --- rcl/include/rcl/node.h | 2 +- rcl/include/rcl/publisher.h | 1 + rcl/include/rcl/service.h | 1 + rcl/include/rcl/subscription.h | 1 + rcl/src/rcl/client.c | 20 ++++++------------ rcl/src/rcl/graph.c | 23 ++++---------------- rcl/src/rcl/node.c | 24 ++++++++++----------- rcl/src/rcl/publisher.c | 27 +++++++++++------------- rcl/src/rcl/service.c | 37 ++++++++++++++++----------------- rcl/src/rcl/subscription.c | 35 +++++++++++-------------------- rcl/test/rcl/test_client.cpp | 2 +- rcl/test/rcl/test_graph.cpp | 6 +++--- rcl/test/rcl/test_node.cpp | 4 ++-- rcl/test/rcl/test_publisher.cpp | 2 +- 14 files changed, 75 insertions(+), 110 deletions(-) diff --git a/rcl/include/rcl/node.h b/rcl/include/rcl/node.h index 3c9e8dc..734e644 100644 --- a/rcl/include/rcl/node.h +++ b/rcl/include/rcl/node.h @@ -191,7 +191,7 @@ rcl_node_init( * * \param[in] node rcl_node_t to be finalized * \return `RCL_RET_OK` if node was finalized successfully, or - * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_NODE_INVALID` if the node pointer is null, or * \return `RCL_RET_ERROR` if an unspecified error occurs. */ RCL_PUBLIC diff --git a/rcl/include/rcl/publisher.h b/rcl/include/rcl/publisher.h index abab352..7222895 100644 --- a/rcl/include/rcl/publisher.h +++ b/rcl/include/rcl/publisher.h @@ -172,6 +172,7 @@ rcl_publisher_init( * \param[in] node handle to the node used to create the publisher * \return `RCL_RET_OK` if publisher was finalized successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_PUBLISHER_INVALID` if the publisher is invalid, or * \return `RCL_RET_NODE_INVALID` if the node is invalid, or * \return `RCL_RET_ERROR` if an unspecified error occurs. */ diff --git a/rcl/include/rcl/service.h b/rcl/include/rcl/service.h index 9bc4ce1..a2fdaaa 100644 --- a/rcl/include/rcl/service.h +++ b/rcl/include/rcl/service.h @@ -176,6 +176,7 @@ rcl_service_init( * \param[in] node handle to the node used to create the service * \return `RCL_RET_OK` if service was deinitialized successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_SERVICE_INVALID` if the service is invalid, or * \return `RCL_RET_NODE_INVALID` if the node is invalid, or * \return `RCL_RET_ERROR` if an unspecified error occurs. */ diff --git a/rcl/include/rcl/subscription.h b/rcl/include/rcl/subscription.h index af588be..7ea1e7c 100644 --- a/rcl/include/rcl/subscription.h +++ b/rcl/include/rcl/subscription.h @@ -177,6 +177,7 @@ rcl_subscription_init( * \param[in] node handle to the node used to create the subscription * \return `RCL_RET_OK` if subscription was deinitialized successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_SUBSCRIPTION_INVALID` if the subscription is invalid, or * \return `RCL_RET_NODE_INVALID` if the node is invalid, or * \return `RCL_RET_ERROR` if an unspecified error occurs. */ diff --git a/rcl/src/rcl/client.c b/rcl/src/rcl/client.c index 5686753..4e4108e 100644 --- a/rcl/src/rcl/client.c +++ b/rcl/src/rcl/client.c @@ -62,9 +62,8 @@ rcl_client_init( rcl_allocator_t * allocator = (rcl_allocator_t *)&options->allocator; RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(service_name, RCL_RET_INVALID_ARGUMENT); @@ -198,13 +197,11 @@ cleanup: rcl_ret_t rcl_client_fini(rcl_client_t * client, rcl_node_t * node) { - (void)node; RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing client"); rcl_ret_t result = RCL_RET_OK; RCL_CHECK_ARGUMENT_FOR_NULL(client, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } if (client->impl) { rcl_allocator_t allocator = client->impl->options.allocator; @@ -268,11 +265,10 @@ rcl_send_request(const rcl_client_t * client, const void * ros_request, int64_t { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Client sending service request"); if (!rcl_client_is_valid(client)) { - return RCL_RET_CLIENT_INVALID; + return RCL_RET_CLIENT_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(ros_request, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL( - sequence_number, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(sequence_number, RCL_RET_INVALID_ARGUMENT); *sequence_number = rcl_atomic_load_int64_t(&client->impl->sequence_number); if (rmw_send_request( client->impl->rmw_handle, ros_request, sequence_number) != RMW_RET_OK) @@ -292,7 +288,7 @@ rcl_take_response( { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Client taking service response"); if (!rcl_client_is_valid(client)) { - return RCL_RET_CLIENT_INVALID; + return RCL_RET_CLIENT_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT); @@ -316,13 +312,9 @@ rcl_take_response( bool rcl_client_is_valid(const rcl_client_t * client) { - const rcl_client_options_t * options; - RCL_CHECK_ARGUMENT_FOR_NULL(client, false); + RCL_CHECK_FOR_NULL_WITH_MSG(client, "client pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( client->impl, "client's rmw implementation is invalid", return false); - options = _client_get_options(client); - RCL_CHECK_FOR_NULL_WITH_MSG( - options, "client's options pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( client->impl->rmw_handle, "client's rmw handle is invalid", return false); return true; diff --git a/rcl/src/rcl/graph.c b/rcl/src/rcl/graph.c index 07e46bc..fcc0b9c 100644 --- a/rcl/src/rcl/graph.c +++ b/rcl/src/rcl/graph.c @@ -36,11 +36,10 @@ rcl_get_topic_names_and_types( bool no_demangle, rcl_names_and_types_t * topic_names_and_types) { - RCL_CHECK_ARGUMENT_FOR_NULL(allocator, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } + RCL_CHECK_ARGUMENT_FOR_NULL(allocator, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(topic_names_and_types, RCL_RET_INVALID_ARGUMENT); rmw_ret_t rmw_ret; rmw_ret = rmw_names_and_types_check_zero(topic_names_and_types); @@ -63,9 +62,8 @@ rcl_get_service_names_and_types( rcl_allocator_t * allocator, rcl_names_and_types_t * service_names_and_types) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(service_names_and_types, RCL_RET_INVALID_ARGUMENT); rmw_ret_t rmw_ret; @@ -97,9 +95,8 @@ rcl_get_node_names( rcutils_string_array_t * node_names, rcutils_string_array_t * node_namespaces) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(node_names, RCL_RET_INVALID_ARGUMENT); if (node_names->size != 0) { @@ -133,10 +130,6 @@ rcl_count_publishers( const char * topic_name, size_t * count) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); - if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; - } const rcl_node_options_t * node_options = rcl_node_get_options(node); if (!node_options) { return RCL_RET_NODE_INVALID; // shouldn't happen, but error is already set if so @@ -153,10 +146,6 @@ rcl_count_subscribers( const char * topic_name, size_t * count) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); - if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; - } const rcl_node_options_t * node_options = rcl_node_get_options(node); if (!node_options) { return RCL_RET_NODE_INVALID; // shouldn't happen, but error is already set if so @@ -173,10 +162,6 @@ rcl_service_server_is_available( const rcl_client_t * client, bool * is_available) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); - if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; - } const rcl_node_options_t * node_options = rcl_node_get_options(node); if (!node_options) { return RCL_RET_NODE_INVALID; // shouldn't happen, but error is already set if so diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 7829b9d..4a5d021 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -411,7 +411,7 @@ rcl_ret_t rcl_node_fini(rcl_node_t * node) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing node"); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_NODE_INVALID); if (!node->impl) { // Repeat calls to fini or calling fini on a zero initialized node is ok. return RCL_RET_OK; @@ -446,9 +446,8 @@ rcl_node_fini(rcl_node_t * node) bool rcl_node_is_valid(const rcl_node_t * node) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, false); - RCL_CHECK_FOR_NULL_WITH_MSG( - node->impl, "rcl node implementation is invalid", return false); + RCL_CHECK_FOR_NULL_WITH_MSG(node, "rcl node pointer is invalid", return false); + RCL_CHECK_FOR_NULL_WITH_MSG(node->impl, "rcl node implementation is invalid", return false); if (node->impl->rcl_instance_id != rcl_get_instance_id()) { RCL_SET_ERROR_MSG("rcl node is invalid, rcl instance id does not match"); return false; @@ -497,7 +496,7 @@ const char * rcl_node_get_name(const rcl_node_t * node) { if (!rcl_node_is_valid(node)) { - return NULL; + return NULL; // error already set } return node->impl->rmw_node_handle->name; } @@ -506,7 +505,7 @@ const char * rcl_node_get_namespace(const rcl_node_t * node) { if (!rcl_node_is_valid(node)) { - return NULL; + return NULL; // error already set } return node->impl->rmw_node_handle->namespace_; } @@ -515,7 +514,7 @@ const rcl_node_options_t * rcl_node_get_options(const rcl_node_t * node) { if (!rcl_node_is_valid(node)) { - return NULL; + return NULL; // error already set } return &node->impl->options; } @@ -523,10 +522,9 @@ rcl_node_get_options(const rcl_node_t * node) rcl_ret_t rcl_node_get_domain_id(const rcl_node_t * node, size_t * domain_id) { - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); const rcl_node_options_t * node_options = rcl_node_get_options(node); if (!node_options) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(domain_id, RCL_RET_INVALID_ARGUMENT); *domain_id = node->impl->actual_domain_id; @@ -537,7 +535,7 @@ rmw_node_t * rcl_node_get_rmw_handle(const rcl_node_t * node) { if (!rcl_node_is_valid(node)) { - return NULL; + return NULL; // error already set } return node->impl->rmw_node_handle; } @@ -545,6 +543,8 @@ rcl_node_get_rmw_handle(const rcl_node_t * node) uint64_t rcl_node_get_rcl_instance_id(const rcl_node_t * node) { + // Not using rcl_node_is_valid() since we can still get the + // instance ID from an initialized node, even if it is invalid RCL_CHECK_ARGUMENT_FOR_NULL(node, 0); RCL_CHECK_FOR_NULL_WITH_MSG(node->impl, "node implementation is invalid", return 0); return node->impl->rcl_instance_id; @@ -554,7 +554,7 @@ const struct rcl_guard_condition_t * rcl_node_get_graph_guard_condition(const rcl_node_t * node) { if (!rcl_node_is_valid(node)) { - return NULL; + return NULL; // error already set } return node->impl->graph_guard_condition; } @@ -563,7 +563,7 @@ const char * rcl_node_get_logger_name(const rcl_node_t * node) { if (!rcl_node_is_valid(node)) { - return NULL; + return NULL; // error already set } return node->impl->logger_name; } diff --git a/rcl/src/rcl/publisher.c b/rcl/src/rcl/publisher.c index 91dfee1..8c0efd2 100644 --- a/rcl/src/rcl/publisher.c +++ b/rcl/src/rcl/publisher.c @@ -64,9 +64,8 @@ rcl_publisher_init( RCL_SET_ERROR_MSG("publisher already initialized, or memory was unintialized"); return RCL_RET_ALREADY_INIT; } - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(topic_name, RCL_RET_INVALID_ARGUMENT); @@ -194,11 +193,11 @@ rcl_ret_t rcl_publisher_fini(rcl_publisher_t * publisher, rcl_node_t * node) { rcl_ret_t result = RCL_RET_OK; - RCL_CHECK_ARGUMENT_FOR_NULL(publisher, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(publisher, RCL_RET_PUBLISHER_INVALID); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } + RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing publisher"); if (publisher->impl) { rcl_allocator_t allocator = publisher->impl->options.allocator; @@ -234,8 +233,9 @@ rcl_publish(const rcl_publisher_t * publisher, const void * ros_message) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Publisher publishing message"); if (!rcl_publisher_is_valid(publisher)) { - return RCL_RET_PUBLISHER_INVALID; + return RCL_RET_PUBLISHER_INVALID; // error already set } + RCL_CHECK_ARGUMENT_FOR_NULL(ros_message, RCL_RET_INVALID_ARGUMENT); if (rmw_publish(publisher->impl->rmw_handle, ros_message) != RMW_RET_OK) { RCL_SET_ERROR_MSG(rmw_get_error_string().str); return RCL_RET_ERROR; @@ -248,8 +248,9 @@ rcl_publish_serialized_message( const rcl_publisher_t * publisher, const rcl_serialized_message_t * serialized_message) { if (!rcl_publisher_is_valid(publisher)) { - return RCL_RET_PUBLISHER_INVALID; + return RCL_RET_PUBLISHER_INVALID; // error already set } + RCL_CHECK_ARGUMENT_FOR_NULL(serialized_message, RCL_RET_INVALID_ARGUMENT); rmw_ret_t ret = rmw_publish_serialized_message(publisher->impl->rmw_handle, serialized_message); if (ret != RMW_RET_OK) { RCL_SET_ERROR_MSG(rmw_get_error_string().str); @@ -265,7 +266,7 @@ const char * rcl_publisher_get_topic_name(const rcl_publisher_t * publisher) { if (!rcl_publisher_is_valid(publisher)) { - return NULL; + return NULL; // error already set } return publisher->impl->rmw_handle->topic_name; } @@ -276,7 +277,7 @@ const rcl_publisher_options_t * rcl_publisher_get_options(const rcl_publisher_t * publisher) { if (!rcl_publisher_is_valid(publisher)) { - return NULL; + return NULL; // error already set } return _publisher_get_options(publisher); } @@ -285,7 +286,7 @@ rmw_publisher_t * rcl_publisher_get_rmw_handle(const rcl_publisher_t * publisher) { if (!rcl_publisher_is_valid(publisher)) { - return NULL; + return NULL; // error already set } return publisher->impl->rmw_handle; } @@ -293,13 +294,9 @@ rcl_publisher_get_rmw_handle(const rcl_publisher_t * publisher) bool rcl_publisher_is_valid(const rcl_publisher_t * publisher) { - const rcl_publisher_options_t * options; - RCL_CHECK_ARGUMENT_FOR_NULL(publisher, false); + RCL_CHECK_FOR_NULL_WITH_MSG(publisher, "publisher pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( publisher->impl, "publisher implementation is invalid", return false); - options = _publisher_get_options(publisher); - RCL_CHECK_FOR_NULL_WITH_MSG( - options, "publisher's options pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( publisher->impl->rmw_handle, "publisher's rmw handle is invalid", return false); return true; diff --git a/rcl/src/rcl/service.c b/rcl/src/rcl/service.c index 4402378..34d131f 100644 --- a/rcl/src/rcl/service.c +++ b/rcl/src/rcl/service.c @@ -59,9 +59,8 @@ rcl_service_init( RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(service_name, RCL_RET_INVALID_ARGUMENT); @@ -204,12 +203,12 @@ rcl_ret_t rcl_service_fini(rcl_service_t * service, rcl_node_t * node) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing service"); - rcl_ret_t result = RCL_RET_OK; - RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(service, RCL_RET_SERVICE_INVALID); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } + + rcl_ret_t result = RCL_RET_OK; if (service->impl) { rcl_allocator_t allocator = service->impl->options.allocator; rmw_node_t * rmw_node = rcl_node_get_rmw_handle(node); @@ -255,7 +254,7 @@ const rcl_service_options_t * rcl_service_get_options(const rcl_service_t * service) { if (!rcl_service_is_valid(service)) { - return NULL; + return NULL; // error already set } return _service_get_options(service); } @@ -264,7 +263,7 @@ rmw_service_t * rcl_service_get_rmw_handle(const rcl_service_t * service) { if (!rcl_service_is_valid(service)) { - return NULL; + return NULL; // error already set } return service->impl->rmw_handle; } @@ -276,11 +275,13 @@ rcl_take_request( void * ros_request) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Service server taking service request"); - const rcl_service_options_t * options = rcl_service_get_options(service); - RCL_CHECK_FOR_NULL_WITH_MSG( - options, "Failed to get service options", return RCL_RET_ERROR); + if (!rcl_service_is_valid(service)) { + return RCL_RET_SERVICE_INVALID; // error already set + } RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(ros_request, RCL_RET_INVALID_ARGUMENT); + const rcl_service_options_t * options = rcl_service_get_options(service); + RCL_CHECK_FOR_NULL_WITH_MSG(options, "Failed to get service options", return RCL_RET_ERROR); bool taken = false; if (rmw_take_request( @@ -304,11 +305,13 @@ rcl_send_response( void * ros_response) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Sending service response"); - const rcl_service_options_t * options = rcl_service_get_options(service); - RCL_CHECK_FOR_NULL_WITH_MSG( - options, "Failed to get service options", return RCL_RET_ERROR); + if (!rcl_service_is_valid(service)) { + return RCL_RET_SERVICE_INVALID; // error already set + } RCL_CHECK_ARGUMENT_FOR_NULL(request_header, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(ros_response, RCL_RET_INVALID_ARGUMENT); + const rcl_service_options_t * options = rcl_service_get_options(service); + RCL_CHECK_FOR_NULL_WITH_MSG(options, "Failed to get service options", return RCL_RET_ERROR); if (rmw_send_response( service->impl->rmw_handle, request_header, ros_response) != RMW_RET_OK) @@ -322,13 +325,9 @@ rcl_send_response( bool rcl_service_is_valid(const rcl_service_t * service) { - const rcl_service_options_t * options; - RCL_CHECK_ARGUMENT_FOR_NULL(service, false); + RCL_CHECK_FOR_NULL_WITH_MSG(service, "service pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( service->impl, "service's implementation is invalid", return false); - options = _service_get_options(service); - RCL_CHECK_FOR_NULL_WITH_MSG( - options, "service's options pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( service->impl->rmw_handle, "service's rmw handle is invalid", return false); return true; diff --git a/rcl/src/rcl/subscription.c b/rcl/src/rcl/subscription.c index 7bf61f2..fcbe954 100644 --- a/rcl/src/rcl/subscription.c +++ b/rcl/src/rcl/subscription.c @@ -57,11 +57,9 @@ rcl_subscription_init( rcl_allocator_t * allocator = (rcl_allocator_t *)&options->allocator; RCL_CHECK_ALLOCATOR_WITH_MSG(allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } - RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(type_support, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(topic_name, RCL_RET_INVALID_ARGUMENT); RCUTILS_LOG_DEBUG_NAMED( @@ -197,10 +195,9 @@ rcl_subscription_fini(rcl_subscription_t * subscription, rcl_node_t * node) { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Finalizing subscription"); rcl_ret_t result = RCL_RET_OK; - RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(node, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(subscription, RCL_RET_SUBSCRIPTION_INVALID); if (!rcl_node_is_valid(node)) { - return RCL_RET_NODE_INVALID; + return RCL_RET_NODE_INVALID; // error already set } if (subscription->impl) { rcl_allocator_t allocator = subscription->impl->options.allocator; @@ -243,8 +240,8 @@ rcl_take( if (!rcl_subscription_is_valid(subscription)) { return RCL_RET_SUBSCRIPTION_INVALID; // error message already set } - RCL_CHECK_ARGUMENT_FOR_NULL( - ros_message, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(ros_message, RCL_RET_INVALID_ARGUMENT); + // If message_info is NULL, use a place holder which can be discarded. rmw_message_info_t dummy_message_info; rmw_message_info_t * message_info_local = message_info ? message_info : &dummy_message_info; @@ -272,7 +269,7 @@ rcl_take_serialized_message( { RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Subscription taking serialized message"); if (!rcl_subscription_is_valid(subscription)) { - return RCL_RET_SUBSCRIPTION_INVALID; // error message already set + return RCL_RET_SUBSCRIPTION_INVALID; // error already set } RCL_CHECK_ARGUMENT_FOR_NULL(serialized_message, RCL_RET_INVALID_ARGUMENT); // If message_info is NULL, use a place holder which can be discarded. @@ -301,7 +298,7 @@ const char * rcl_subscription_get_topic_name(const rcl_subscription_t * subscription) { if (!rcl_subscription_is_valid(subscription)) { - return NULL; + return NULL; // error already set } return subscription->impl->rmw_handle->topic_name; } @@ -312,7 +309,7 @@ const rcl_subscription_options_t * rcl_subscription_get_options(const rcl_subscription_t * subscription) { if (!rcl_subscription_is_valid(subscription)) { - return NULL; + return NULL; // error already set } return _subscription_get_options(subscription); } @@ -321,7 +318,7 @@ rmw_subscription_t * rcl_subscription_get_rmw_handle(const rcl_subscription_t * subscription) { if (!rcl_subscription_is_valid(subscription)) { - return NULL; + return NULL; // error already set } return subscription->impl->rmw_handle; } @@ -329,19 +326,11 @@ rcl_subscription_get_rmw_handle(const rcl_subscription_t * subscription) bool rcl_subscription_is_valid(const rcl_subscription_t * subscription) { - const rcl_subscription_options_t * options; - RCL_CHECK_ARGUMENT_FOR_NULL(subscription, false); + RCL_CHECK_FOR_NULL_WITH_MSG(subscription, "subscription pointer is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( - subscription->impl, - "subscription's implementation is invalid", - return false); - options = _subscription_get_options(subscription); + subscription->impl, "subscription's implementation is invalid", return false); RCL_CHECK_FOR_NULL_WITH_MSG( - options, "subscription's option pointer is invalid", return false); - RCL_CHECK_FOR_NULL_WITH_MSG( - subscription->impl->rmw_handle, - "subscription's rmw handle is invalid", - return false); + subscription->impl->rmw_handle, "subscription's rmw handle is invalid", return false); return true; } diff --git a/rcl/test/rcl/test_client.cpp b/rcl/test/rcl/test_client.cpp index 63a6376..e895a06 100644 --- a/rcl/test/rcl/test_client.cpp +++ b/rcl/test/rcl/test_client.cpp @@ -110,7 +110,7 @@ TEST_F(TestClientFixture, test_client_init_fini) { // Try passing null for a node pointer in init. client = rcl_get_zero_initialized_client(); ret = rcl_client_init(&client, nullptr, ts, topic_name, &default_client_options); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str; + EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; rcl_reset_error(); // Check if null publisher is valid diff --git a/rcl/test/rcl/test_graph.cpp b/rcl/test/rcl/test_graph.cpp index 5f43236..4b95ab2 100644 --- a/rcl/test/rcl/test_graph.cpp +++ b/rcl/test/rcl/test_graph.cpp @@ -115,7 +115,7 @@ TEST_F( rcl_node_t zero_node = rcl_get_zero_initialized_node(); // invalid node ret = rcl_get_topic_names_and_types(nullptr, &allocator, false, &tnat); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str; + EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; rcl_reset_error(); ret = rcl_get_topic_names_and_types(&zero_node, &allocator, false, &tnat); EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; @@ -156,7 +156,7 @@ TEST_F( size_t count; // invalid node ret = rcl_count_publishers(nullptr, topic_name, &count); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str; + EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; rcl_reset_error(); ret = rcl_count_publishers(&zero_node, topic_name, &count); EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; @@ -193,7 +193,7 @@ TEST_F( size_t count; // invalid node ret = rcl_count_subscribers(nullptr, topic_name, &count); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str; + EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; rcl_reset_error(); ret = rcl_count_subscribers(&zero_node, topic_name, &count); EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; diff --git a/rcl/test/rcl/test_node.cpp b/rcl/test/rcl/test_node.cpp index d8e717d..6236cad 100644 --- a/rcl/test/rcl/test_node.cpp +++ b/rcl/test/rcl/test_node.cpp @@ -221,7 +221,7 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_accessors) // Test rcl_node_get_domain_id(). size_t actual_domain_id; ret = rcl_node_get_domain_id(nullptr, &actual_domain_id); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); + EXPECT_EQ(RCL_RET_NODE_INVALID, ret); ASSERT_TRUE(rcl_error_is_set()); rcl_reset_error(); ret = rcl_node_get_domain_id(&zero_node, &actual_domain_id); @@ -347,7 +347,7 @@ TEST_F(CLASSNAME(TestNodeFixture, RMW_IMPLEMENTATION), test_rcl_node_life_cycle) // Try fini with invalid arguments. ret = rcl_node_fini(nullptr); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << "Expected RCL_RET_INVALID_ARGUMENT"; + EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << "Expected RCL_RET_NODE_INVALID"; ASSERT_TRUE(rcl_error_is_set()); rcl_reset_error(); // Try fini with an uninitialized node. diff --git a/rcl/test/rcl/test_publisher.cpp b/rcl/test/rcl/test_publisher.cpp index b5071c3..cfce023 100644 --- a/rcl/test/rcl/test_publisher.cpp +++ b/rcl/test/rcl/test_publisher.cpp @@ -191,7 +191,7 @@ TEST_F(CLASSNAME(TestPublisherFixture, RMW_IMPLEMENTATION), test_publisher_init_ // Try passing null for a node pointer in init. publisher = rcl_get_zero_initialized_publisher(); ret = rcl_publisher_init(&publisher, nullptr, ts, topic_name, &default_publisher_options); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str; + EXPECT_EQ(RCL_RET_NODE_INVALID, ret) << rcl_get_error_string().str; rcl_reset_error(); // Try passing an invalid (uninitialized) node in init.