CLI remapping followup (#221)

* Document graph functions don't remap

* Prettier if statements using rmw_validate_*

* all arguments on one line

* Whe -> The

* Check args for NULL

* Fix topic remap being interpretted as namespace remap

rcl/include/rcl/arguments.h

@@ -64,7 +64,7 @@ rcl_get_zero_initialized_arguments(void);
  * Lock-Free          | Yes
  *
  * \param[in] argc The number of arguments in argv.
- * \param[in] argv Whe values of the arguments.
+ * \param[in] argv The values of the arguments.
  * \param[in] allocator A valid allocator.
  * \param[out] args_output A structure that will contain the result of parsing.
  * \return `RCL_RET_OK` if the arguments were parsed successfully, or

rcl/include/rcl/graph.h

@@ -56,6 +56,10 @@ typedef rmw_names_and_types_t rcl_names_and_types_t;
  *
  * \see rmw_get_topic_names_and_types for more details on no_demangle
  *
+ * The returned names are not automatically remapped by this function.
+ * Attempting to create publishers or subscribers using names returned by this function may not
+ * result in the desired topic name being used depending on the remap rules in use.
+ *
  * <hr>
  * Attribute          | Adherence
  * ------------------ | -------------
@@ -96,6 +100,10 @@ rcl_get_topic_names_and_types(
  * it is no longer needed.
  * Failing to do so will result in leaked memory.
  *
+ * The returned names are not automatically remapped by this function.
+ * Attempting to create clients or services using names returned by this function may not result in
+ * the desired service name being used depending on the remap rules in use.
+ *
  * <hr>
  * Attribute          | Adherence
  * ------------------ | -------------
@@ -219,6 +227,12 @@ rcl_get_node_names(
  * In the event that error handling needs to allocate memory, this function
  * will try to use the node's allocator.
  *
+ * The topic name is not automatically remapped by this function.
+ * If there is a publisher created with topic name `foo` and remap rule `foo:=bar` then calling
+ * this with `topic_name` set to `bar` will return a count of 1, and with `topic_name` set to `foo`
+ * will return a count of 0.
+ * /sa rcl_remap_topic_name()
+ *
  * <hr>
  * Attribute          | Adherence
  * ------------------ | -------------
@@ -260,6 +274,12 @@ rcl_count_publishers(
  * In the event that error handling needs to allocate memory, this function
  * will try to use the node's allocator.
  *
+ * The topic name is not automatically remapped by this function.
+ * If there is a subscriber created with topic name `foo` and remap rule `foo:=bar` then calling
+ * this with `topic_name` set to `bar` will return a count of 1, and with `topic_name` set to `foo`
+ * will return a count of 0.
+ * /sa rcl_remap_topic_name()
+ *
  * <hr>
  * Attribute          | Adherence
  * ------------------ | -------------

rcl/src/rcl/arguments.c

@@ -50,6 +50,9 @@ _rcl_parse_remap_rule(
   rcl_allocator_t allocator,
   rcl_remap_t * output_rule)
 {
+  RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT, allocator);
+  RCL_CHECK_ARGUMENT_FOR_NULL(output_rule, RCL_RET_INVALID_ARGUMENT, allocator);
+
   size_t len_node_name = 0;
   size_t len_match = 0;
   size_t len_replacement = 0;
@@ -108,14 +111,12 @@ _rcl_parse_remap_rule(
   if (len_node_name) {
     int validation_result;
     size_t invalid_index;
-    if (
-      RMW_RET_OK != rmw_validate_node_name_with_size(arg, len_node_name, &validation_result,
-      &invalid_index))
-    {
+    rmw_ret_t rmw_ret = rmw_validate_node_name_with_size(
+      arg, len_node_name, &validation_result, &invalid_index);
+    if (RMW_RET_OK != rmw_ret) {
       RCL_SET_ERROR_MSG("failed to run check on node name", allocator);
       return RCL_RET_ERROR;
-    }
-    if (RMW_NODE_NAME_VALID != validation_result) {
+    } else if (RMW_NODE_NAME_VALID != validation_result) {
       RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
         allocator,
         "node name prefix invalid: %s", rmw_node_name_validation_result_string(validation_result));
@@ -125,9 +126,9 @@ _rcl_parse_remap_rule(
 
   // Figure out what type of rule this is, default is to apply to topic and service names
   rcl_remap_type_t type = RCL_TOPIC_REMAP | RCL_SERVICE_REMAP;
-  if (0 == strncmp("__ns", match_begin, len_match)) {
+  if (4 == len_match && 0 == strncmp("__ns", match_begin, len_match)) {
     type = RCL_NAMESPACE_REMAP;
-  } else if (0 == strncmp("__node", match_begin, len_match)) {
+  } else if (6 == len_match && 0 == strncmp("__node", match_begin, len_match)) {
     type = RCL_NODENAME_REMAP;
   }
 
@@ -158,11 +159,11 @@ _rcl_parse_remap_rule(
   } else if (RCL_NAMESPACE_REMAP == type) {
     int validation_result;
     size_t invalid_idx;
-    if (RMW_RET_OK != rmw_validate_namespace(replacement_begin, &validation_result, &invalid_idx)) {
+    rmw_ret_t rmw_ret = rmw_validate_namespace(replacement_begin, &validation_result, &invalid_idx);
+    if (RMW_RET_OK != rmw_ret) {
       RCL_SET_ERROR_MSG("failed to run check on namespace", allocator);
       return RCL_RET_ERROR;
-    }
-    if (RMW_NAMESPACE_VALID != validation_result) {
+    } else if (RMW_NAMESPACE_VALID != validation_result) {
       RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
         allocator,
         "namespace is invalid: %s", rmw_namespace_validation_result_string(validation_result));
@@ -171,11 +172,11 @@ _rcl_parse_remap_rule(
   } else if (RCL_NODENAME_REMAP == type) {
     int validation_result;
     size_t invalid_idx;
-    if (RMW_RET_OK != rmw_validate_node_name(replacement_begin, &validation_result, &invalid_idx)) {
+    rmw_ret_t rmw_ret = rmw_validate_node_name(replacement_begin, &validation_result, &invalid_idx);
+    if (RMW_RET_OK != rmw_ret) {
       RCL_SET_ERROR_MSG("failed to run check on node name", allocator);
       return RCL_RET_ERROR;
-    }
-    if (RMW_NODE_NAME_VALID != validation_result) {
+    } else if (RMW_NODE_NAME_VALID != validation_result) {
       RCL_SET_ERROR_MSG_WITH_FORMAT_STRING(
         allocator,
         "node name is invalid: %s", rmw_node_name_validation_result_string(validation_result));

rcl/test/rcl/test_arguments.cpp

@@ -87,6 +87,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), check_valid_vs_inval
   EXPECT_TRUE(is_valid_arg("__node:=nodename123"));
   EXPECT_TRUE(is_valid_arg("__ns:=/foo/bar"));
   EXPECT_TRUE(is_valid_arg("__ns:=/"));
+  EXPECT_TRUE(is_valid_arg("_:=kq"));
   EXPECT_TRUE(is_valid_arg("nodename:__ns:=/foobar"));
   EXPECT_TRUE(is_valid_arg("foo:=bar"));
   EXPECT_TRUE(is_valid_arg("~/foo:=~/bar"));