-
Notifications
You must be signed in to change notification settings - Fork 33
Conversation
This patch affects: - rmw_get_node_names() - rmw_get_node_names_with_enclaves() - rmw_get_topic_names_and_types() - rmw_get_service_names_and_types() - rmw_get_publishers_info_by_topic() - rmw_get_subscriptions_info_by_topic() - rmw_get_subscriber_names_and_types_by_node() - rmw_get_publisher_names_and_types_by_node() - rmw_get_service_names_and_types_by_node() - rmw_get_client_names_and_types_by_node() - rmw_count_publishers() - rmw_count_subscribers() Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
if (RMW_RET_OK != rmw_check_zero_rmw_string_array(node_names)) { | ||
return RMW_RET_INVALID_ARGUMENT; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems inconsistent with:
ret = rmw_names_and_types_check_zero(topic_names_and_types);
if (RMW_RET_OK != ret) {
return ret;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess is that rmw_check_zero_rmw_string_array(node_names)
does not return RMW_RET_INVALID_ARGUMENT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, it's a bit weird the semantic different between the two, but it doesn't matter much,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. rmw_check_zero_rmw_string_array
returns RMW_RET_INVALID_ARGUMENT
if the argument is NULL, and RMW_RET_ERROR
if it's not zero initialized. We can change rmw_names_and_types_check_zero
call sites to the do the same for consistency's sake though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Alright, going in! Thanks for all the good reviews! |
This patch affects: - rmw_get_node_names() - rmw_get_node_names_with_enclaves() - rmw_get_topic_names_and_types() - rmw_get_service_names_and_types() - rmw_get_publishers_info_by_topic() - rmw_get_subscriptions_info_by_topic() - rmw_get_subscriber_names_and_types_by_node() - rmw_get_publisher_names_and_types_by_node() - rmw_get_service_names_and_types_by_node() - rmw_get_client_names_and_types_by_node() - rmw_count_publishers() - rmw_count_subscribers() Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
This patch affects: - rmw_get_node_names() - rmw_get_node_names_with_enclaves() - rmw_get_topic_names_and_types() - rmw_get_service_names_and_types() - rmw_get_publishers_info_by_topic() - rmw_get_subscriptions_info_by_topic() - rmw_get_subscriber_names_and_types_by_node() - rmw_get_publisher_names_and_types_by_node() - rmw_get_service_names_and_types_by_node() - rmw_get_client_names_and_types_by_node() - rmw_count_publishers() - rmw_count_subscribers() Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Connected to ros2/rmw#272. This patch affects:
rmw_get_node_names()
rmw_get_node_names_with_enclaves()
rmw_get_topic_names_and_types()
rmw_get_service_names_and_types()
rmw_get_publishers_info_by_topic()
rmw_get_subscriptions_info_by_topic()
rmw_get_subscriber_names_and_types_by_node()
rmw_get_publisher_names_and_types_by_node()
rmw_get_service_names_and_types_by_node()
rmw_get_client_names_and_types_by_node()
rmw_count_publishers()
rmw_count_subscribers()