rcl_action: address various clang static analysis fixes (ros2#864)

* Address various clang static analysis fixes Signed-off-by: Stephen Brawner <brawner@gmail.com> * Add gtest assert Signed-off-by: Stephen Brawner <brawner@gmail.com> * Taking array by reference Signed-off-by: Stephen Brawner <brawner@gmail.com>
ApexAI · Nov 30, 2020 · 57d7b74 · 57d7b74
1 parent 5093291
commit 57d7b74
Show file tree

Hide file tree

Showing 3 changed files with 18 additions and 4 deletions.
diff --git a/rcl_action/src/rcl_action/action_server.c b/rcl_action/src/rcl_action/action_server.c
@@ -656,13 +656,17 @@ rcl_action_expire_goals(
       }
     }
   }
-  ret_final = _recalculate_expire_timer(
+  rcl_ret_t expire_timer_ret = _recalculate_expire_timer(
     &action_server->impl->expire_timer,
     action_server->impl->options.result_timeout.nanoseconds,
     action_server->impl->goal_handles,
     action_server->impl->num_goal_handles,
     action_server->impl->clock);
 
+  if (RCL_RET_OK != expire_timer_ret) {
+    ret_final = expire_timer_ret;
+  }
+
   // If argument is not null, then set it
   if (NULL != num_expired) {
     (*num_expired) = num_goals_expired;
@@ -795,8 +799,9 @@ rcl_action_process_cancel_request(
   if (RCL_RET_OK != ret) {
     if (RCL_RET_BAD_ALLOC == ret) {
       ret_final = RCL_RET_BAD_ALLOC;  // error already set
+    } else {
+      ret_final = RCL_RET_ERROR;  // error already set
     }
-    ret_final = RCL_RET_ERROR;  // error already set
     goto cleanup;
   }
 

diff --git a/rcl_action/test/rcl_action/test_action_server.cpp b/rcl_action/test/rcl_action/test_action_server.cpp
@@ -727,7 +727,11 @@ TEST_F(TestActionServer, test_action_server_get_goal_status_array)
   EXPECT_NE(status_array.msg.status_list.data, nullptr);
   EXPECT_EQ(status_array.msg.status_list.size, 1u);
   rcl_action_goal_info_t * goal_info_out = &status_array.msg.status_list.data[0].goal_info;
-  EXPECT_TRUE(uuidcmp(goal_info_out->goal_id.uuid, goal_info_in.goal_id.uuid));
+
+  // Passing this array by pointer can confuse scan-build into thinking the pointer is
+  // not checked for null. Passing by reference works fine
+  const auto & goal_info_out_uuid = goal_info_out->goal_id.uuid;
+  EXPECT_TRUE(uuidcmp(goal_info_out_uuid, goal_info_in.goal_id.uuid));
   ret = rcl_action_goal_status_array_fini(&status_array);
   EXPECT_EQ(ret, RCL_RET_OK);
 
@@ -870,7 +874,11 @@ TEST_F(TestActionServerCancelPolicy, test_action_process_cancel_request_single_g
     ASSERT_EQ(cancel_response.msg.goals_canceling.size, 1u);
     EXPECT_EQ(cancel_response.msg.return_code, action_msgs__srv__CancelGoal_Response__ERROR_NONE);
     rcl_action_goal_info_t * goal_info = &cancel_response.msg.goals_canceling.data[0];
-    EXPECT_TRUE(uuidcmp(goal_info->goal_id.uuid, cancel_request.goal_info.goal_id.uuid));
+
+    // Passing this array by pointer can confuse scan-build into thinking the pointer is
+    // not checked for null. Passing by reference works fine
+    const auto & goal_info_uuid = goal_info->goal_id.uuid;
+    EXPECT_TRUE(uuidcmp(goal_info_uuid, cancel_request.goal_info.goal_id.uuid));
     EXPECT_EQ(RCL_RET_OK, rcl_action_cancel_response_fini(&cancel_response));
   }
   {

diff --git a/rcl_action/test/rcl_action/test_wait.cpp b/rcl_action/test/rcl_action/test_wait.cpp
@@ -796,6 +796,7 @@ TEST_F(TestActionServerWait, test_server_wait_set_get_entities_ready) {
   EXPECT_EQ(ret, RCL_RET_INVALID_ARGUMENT);
 
   ret = rcl_wait_set_init(&wait_set, 0, 0, 1, 0, 3, 0, &this->context, rcl_get_default_allocator());
+  ASSERT_EQ(ret, RCL_RET_OK) << rcl_get_error_string().str;
   wait_set.services[0] = &this->action_server.impl->goal_service;
   this->action_server.impl->wait_set_goal_service_index = 0;
   wait_set.services[1] = &this->action_server.impl->cancel_service;