Warn unused (#327)

* comply with unused warnings

* fix flakiness and add test for transitions

* mark flaky test

* duplicate const char * in State constructor

* linters

* correct year in license

* mark flaky test

rclcpp_lifecycle/CMakeLists.txt

@@ -79,6 +79,15 @@ if(BUILD_TESTING)
     )
     target_link_libraries(test_callback_exceptions ${PROJECT_NAME})
   endif()
+  ament_add_gtest(test_state_wrapper test/test_state_wrapper.cpp)
+  if(TARGET test_state_wrapper)
+    target_include_directories(test_state_wrapper PUBLIC
+      ${rcl_lifecycle_INCLUDE_DIRS}
+      ${rclcpp_INCLUDE_DIRS}
+      ${rclcpp_lifecycle_INCLUDE_DIRS}
+    )
+    target_link_libraries(test_state_wrapper ${PROJECT_NAME})
+  endif()
 endif()
 
 ament_export_dependencies(rclcpp)

rclcpp_lifecycle/src/lifecycle_node_interface_impl.hpp

@@ -63,9 +63,15 @@ class LifecycleNode::LifecycleNodeInterfaceImpl
     if (rcl_lifecycle_state_machine_is_initialized(&state_machine_) != RCL_RET_OK) {
       fprintf(stderr, "%s:%u, FATAL: rcl_state_machine got destroyed externally.\n",
         __FILE__, __LINE__);
+      // TODO(karsten1987): Remove else. It should always be possible to call
+      // fini on state machine, also when not initialized
     } else {
-      rcl_lifecycle_state_machine_fini(&state_machine_,
-        node_base_interface_->get_rcl_node_handle());
+      if (rcl_lifecycle_state_machine_fini(&state_machine_,
+        node_base_interface_->get_rcl_node_handle()) != RCL_RET_OK)
+      {
+        fprintf(stderr, "%s:%u, FATAL: failed to destroy rcl_state_machine.\n",
+          __FILE__, __LINE__);
+      }
     }
   }
 
@@ -304,16 +310,20 @@ class LifecycleNode::LifecycleNodeInterfaceImpl
       rcl_lifecycle_ret_t error_resolved = execute_callback(state_machine_.current_state->id,
           initial_state);
       if (error_resolved == RCL_RET_OK) {
-        fprintf(stderr, "Exception handling was successful\n");
+        // fprintf(stderr, "Exception handling was successful\n");
         // We call cleanup on the error state
-        rcl_lifecycle_trigger_transition(
-          &state_machine_, error_resolved, true);
-        fprintf(stderr, "current state after error callback%s\n",
-          state_machine_.current_state->label);
+        if (rcl_lifecycle_trigger_transition(&state_machine_, error_resolved, true) != RCL_RET_OK) {
+          fprintf(stderr, "Failed to call cleanup on error state\n");
+          return false;
+        }
+        // fprintf(stderr, "current state after error callback%s\n",
+        //  state_machine_.current_state->label);
       } else {
         // We call shutdown on the error state
-        rcl_lifecycle_trigger_transition(
-          &state_machine_, error_resolved, true);
+        if (rcl_lifecycle_trigger_transition(&state_machine_, error_resolved, true) != RCL_RET_OK) {
+          fprintf(stderr, "Failed to call cleanup on error state\n");
+          return false;
+        }
       }
     }
     // This true holds in both cases where the actual callback

rclcpp_lifecycle/src/state.cpp

@@ -15,6 +15,9 @@
 #include "rclcpp_lifecycle/state.hpp"
 
 #include <lifecycle_msgs/msg/state.hpp>
+
+#include <rcutils/allocator.h>
+#include <rcutils/strdup.h>
 #include <rcl_lifecycle/data_types.h>
 
 #include <string>
@@ -35,7 +38,8 @@ State::State(uint8_t id, const std::string & label)
 
   auto state_handle = new rcl_lifecycle_state_t;
   state_handle->id = id;
-  state_handle->label = label.c_str();
+  state_handle->label =
+    rcutils_strndup(label.c_str(), label.size(), rcutils_get_default_allocator());
 
   state_handle_ = state_handle;
 }
@@ -56,12 +60,18 @@ State::~State()
 uint8_t
 State::id() const
 {
+  if (!state_handle_) {
+    throw std::runtime_error("Error in state! Internal state_handle is NULL.");
+  }
   return state_handle_->id;
 }
 
 std::string
 State::label() const
 {
+  if (!state_handle_) {
+    throw std::runtime_error("Error in state! Internal state_handle is NULL.");
+  }
   return state_handle_->label;
 }
 

rclcpp_lifecycle/src/transition.cpp

@@ -90,17 +90,13 @@ Transition::label() const
 State
 Transition::start_state() const
 {
-  return State(
-    transition_handle_->start->id,
-    transition_handle_->start->label);
+  return State(transition_handle_->start);
 }
 
 State
 Transition::goal_state() const
 {
-  return State(
-    transition_handle_->goal->id,
-    transition_handle_->goal->label);
+  return State(transition_handle_->goal);
 }
 
 }  // namespace rclcpp_lifecycle

rclcpp_lifecycle/test/test_state_wrapper.cpp

@@ -0,0 +1,77 @@
+// Copyright 2017 Open Source Robotics Foundation, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include <gtest/gtest.h>
+#include <memory>
+#include <string>
+#include <utility>
+#include <vector>
+
+#include "rclcpp_lifecycle/lifecycle_node.hpp"
+
+class TestStateWrapper : public ::testing::Test
+{
+protected:
+  static void SetUpTestCase()
+  {
+  }
+};
+
+TEST_F(TestStateWrapper, wrapper) {
+  {
+    rclcpp_lifecycle::State state(1, "my_state");
+    EXPECT_EQ(1, state.id());
+    EXPECT_FALSE(state.label().empty());
+    EXPECT_STREQ("my_state", state.label().c_str());
+  }
+
+  {
+    rcl_lifecycle_state_t lc_state = {"my_c_state", 2, NULL, NULL, 0};
+    rclcpp_lifecycle::State c_state(lc_state.id, lc_state.label);
+    EXPECT_EQ(2, c_state.id());
+    EXPECT_FALSE(c_state.label().empty());
+    EXPECT_STREQ("my_c_state", c_state.label().c_str());
+  }
+
+  {
+    rcl_lifecycle_state_t lc_state = {"my_c_state", 2, NULL, NULL, 0};
+    rclcpp_lifecycle::State c_state(&lc_state);
+    EXPECT_EQ(2, c_state.id());
+    EXPECT_FALSE(c_state.label().empty());
+    EXPECT_STREQ("my_c_state", c_state.label().c_str());
+  }
+
+  {
+    rcl_lifecycle_state_t * lc_state =
+      new rcl_lifecycle_state_t {"my_c_state", 3, NULL, NULL, 0};
+    rclcpp_lifecycle::State c_state(lc_state->id, lc_state->label);
+    EXPECT_EQ(3, c_state.id());
+    EXPECT_FALSE(c_state.label().empty());
+    EXPECT_STREQ("my_c_state", c_state.label().c_str());
+  }
+
+
+  // introduces flakiness
+  // unsupported behavior!
+  // fails when compiled with memory sanitizer
+  // {
+  //  rcl_lifecycle_state_t * lc_state
+  //    = new rcl_lifecycle_state_t {"my_c_state", 3, NULL, NULL, 0};
+  //  rclcpp_lifecycle::State c_state(lc_state);
+  //  delete lc_state;
+  //  lc_state = NULL;
+  //  EXPECT_EQ(3, c_state.id());
+  //  EXPECT_STREQ("my_c_state", c_state.label().c_str());
+  // }
+}