[rcl] Remove redundant error checking and set explicit error message with '_is_valid' functions

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

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.
  */

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.
  */

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.
  */

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(
+  RCL_CHECK_ARGUMENT_FOR_NULL(sequence_number, RCL_RET_INVALID_ARGUMENT);
-    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_FOR_NULL_WITH_MSG(client, "client pointer is invalid", return false);
-  RCL_CHECK_ARGUMENT_FOR_NULL(client, 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;

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

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, "rcl node pointer is invalid", return false);
-  RCL_CHECK_FOR_NULL_WITH_MSG(
+  RCL_CHECK_FOR_NULL_WITH_MSG(node->impl, "rcl node implementation is invalid", return false);
-    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;
 }

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(publisher, RCL_RET_PUBLISHER_INVALID);
-  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
   }
+
   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_FOR_NULL_WITH_MSG(publisher, "publisher pointer is invalid", return false);
-  RCL_CHECK_ARGUMENT_FOR_NULL(publisher, 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;

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_SERVICE_INVALID);
-  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_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);
+  if (!rcl_service_is_valid(service)) {
-  RCL_CHECK_FOR_NULL_WITH_MSG(
+    return RCL_RET_SERVICE_INVALID;  // error already set
-    options, "Failed to get service options", return RCL_RET_ERROR);
+  }
   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);
+  if (!rcl_service_is_valid(service)) {
-  RCL_CHECK_FOR_NULL_WITH_MSG(
+    return RCL_RET_SERVICE_INVALID;  // error already set
-    options, "Failed to get service options", return RCL_RET_ERROR);
+  }
   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_FOR_NULL_WITH_MSG(service, "service pointer is invalid", return false);
-  RCL_CHECK_ARGUMENT_FOR_NULL(service, 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;

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(subscription, RCL_RET_SUBSCRIPTION_INVALID);
-  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 (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(
+  RCL_CHECK_ARGUMENT_FOR_NULL(ros_message, RCL_RET_INVALID_ARGUMENT);
-    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_FOR_NULL_WITH_MSG(subscription, "subscription pointer is invalid", return false);
-  RCL_CHECK_ARGUMENT_FOR_NULL(subscription, false);
   RCL_CHECK_FOR_NULL_WITH_MSG(
-    subscription->impl,
+    subscription->impl, "subscription's implementation is invalid", return false);
-    "subscription's implementation is invalid",
-    return false);
-  options = _subscription_get_options(subscription);
   RCL_CHECK_FOR_NULL_WITH_MSG(
-    options, "subscription's option pointer is invalid", return false);
+    subscription->impl->rmw_handle, "subscription's rmw handle is invalid", return false);
-  RCL_CHECK_FOR_NULL_WITH_MSG(
-    subscription->impl->rmw_handle,
-    "subscription's rmw handle is invalid",
-    return false);
   return true;
 }
 

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

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;

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.

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.