Fix conversion of negative durations to messages (#1188) (#1371)

* Fix conversion from negative Duration or Time to the respective message type and throw in Duration::to_rmw_time() if the duration is negative.
rmw_time_t cannot represent negative durations.

Constructors and assignment operators can be just defaulted.

Other changes are mainly cosmetical, to make conversions between signed
and unsigned types and between 32-bit and 64-bit types more explicit.

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

* Add -Wconversion compiler option and fix implicit conversions that might alter the value

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

* Fix usage of fixture class in some unit tests by using gtest macro TEST_F() instead of TEST().

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

* Add compiler option -Wno-sign-conversion to fix build with Clang on macOS

Signed-off-by: Johannes Meyer <johannes@intermodalics.eu>

Co-authored-by: Johannes Meyer <johannes@intermodalics.eu>

rclcpp/CMakeLists.txt

@@ -25,7 +25,11 @@ if(NOT CMAKE_CXX_STANDARD)
   set(CMAKE_CXX_STANDARD 14)
 endif()
 if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-  add_compile_options(-Wall -Wextra -Wpedantic -Wnon-virtual-dtor -Woverloaded-virtual)
+  # About -Wno-sign-conversion: With Clang, -Wconversion implies -Wsign-conversion. There are a number of
+  # implicit sign conversions in rclcpp and gtest.cc, see https://ci.ros2.org/job/ci_osx/9265/.
+  # Hence disabling -Wsign-conversion for now until all those have eventually been fixed.
+  # (from https://github.com/ros2/rclcpp/pull/1188#issuecomment-650229140)
+  add_compile_options(-Wall -Wextra -Wconversion -Wno-sign-conversion -Wpedantic -Wnon-virtual-dtor -Woverloaded-virtual)
 endif()
 
 set(${PROJECT_NAME}_SRCS

rclcpp/include/rclcpp/duration.hpp

@@ -72,7 +72,7 @@ public:
   operator=(const Duration & rhs);
 
   Duration &
-  operator=(const builtin_interfaces::msg::Duration & Duration_msg);
+  operator=(const builtin_interfaces::msg::Duration & duration_msg);
 
   bool
   operator==(const rclcpp::Duration & rhs) const;

rclcpp/include/rclcpp/time.hpp

@@ -48,10 +48,10 @@ public:
   /// Time constructor
   /**
    * \param nanoseconds since time epoch
-   * \param clock clock type
+   * \param clock_type clock type
    */
   RCLCPP_PUBLIC
-  explicit Time(int64_t nanoseconds = 0, rcl_clock_type_t clock = RCL_SYSTEM_TIME);
+  explicit Time(int64_t nanoseconds = 0, rcl_clock_type_t clock_type = RCL_SYSTEM_TIME);
 
   /// Copy constructor
   RCLCPP_PUBLIC
@@ -60,13 +60,13 @@ public:
   /// Time constructor
   /**
    * \param time_msg builtin_interfaces time message to copy
-   * \param ros_time clock type
+   * \param clock_type clock type
    * \throws std::runtime_error if seconds are negative
    */
   RCLCPP_PUBLIC
   Time(
     const builtin_interfaces::msg::Time & time_msg,
-    rcl_clock_type_t ros_time = RCL_ROS_TIME);
+    rcl_clock_type_t clock_type = RCL_ROS_TIME);
 
   /// Time constructor
   /**
@@ -90,6 +90,12 @@ public:
   Time &
   operator=(const Time & rhs);
 
+  /**
+   * Assign Time from a builtin_interfaces::msg::Time instance.
+   * The clock_type will be reset to RCL_ROS_TIME.
+   * Equivalent to *this = Time(time_msg, RCL_ROS_TIME).
+   * \throws std::runtime_error if seconds are negative
+   */
   RCLCPP_PUBLIC
   Time &
   operator=(const builtin_interfaces::msg::Time & time_msg);

rclcpp/src/rclcpp/duration.cpp

@@ -47,17 +47,14 @@ Duration::Duration(std::chrono::nanoseconds nanoseconds)
   rcl_duration_.nanoseconds = nanoseconds.count();
 }
 
-Duration::Duration(const Duration & rhs)
+Duration::Duration(const Duration & rhs) = default;
-{
-  rcl_duration_.nanoseconds = rhs.rcl_duration_.nanoseconds;
-}
 
 Duration::Duration(
   const builtin_interfaces::msg::Duration & duration_msg)
 {
   rcl_duration_.nanoseconds =
-    static_cast<rcl_duration_value_t>(RCL_S_TO_NS(duration_msg.sec));
+    RCL_S_TO_NS(static_cast<rcl_duration_value_t>(duration_msg.sec));
-  rcl_duration_.nanoseconds += duration_msg.nanosec;
+  rcl_duration_.nanoseconds += static_cast<rcl_duration_value_t>(duration_msg.nanosec);
 }
 
 Duration::Duration(const rcl_duration_t & duration)
@@ -69,24 +66,25 @@ Duration::Duration(const rcl_duration_t & duration)
 Duration::operator builtin_interfaces::msg::Duration() const
 {
   builtin_interfaces::msg::Duration msg_duration;
-  msg_duration.sec = static_cast<std::int32_t>(RCL_NS_TO_S(rcl_duration_.nanoseconds));
+  constexpr rcl_duration_value_t kDivisor = RCL_S_TO_NS(1);
-  msg_duration.nanosec =
+  const auto result = std::div(rcl_duration_.nanoseconds, kDivisor);
-    static_cast<std::uint32_t>(rcl_duration_.nanoseconds % (1000 * 1000 * 1000));
+  if (result.rem >= 0) {
+    msg_duration.sec = static_cast<std::int32_t>(result.quot);
+    msg_duration.nanosec = static_cast<std::uint32_t>(result.rem);
+  } else {
+    msg_duration.sec = static_cast<std::int32_t>(result.quot - 1);
+    msg_duration.nanosec = static_cast<std::uint32_t>(kDivisor + result.rem);
+  }
   return msg_duration;
 }
 
 Duration &
-Duration::operator=(const Duration & rhs)
+Duration::operator=(const Duration & rhs) = default;
-{
-  rcl_duration_.nanoseconds = rhs.rcl_duration_.nanoseconds;
-  return *this;
-}
 
 Duration &
 Duration::operator=(const builtin_interfaces::msg::Duration & duration_msg)
 {
-  rcl_duration_.nanoseconds = RCL_S_TO_NS(static_cast<int64_t>(duration_msg.sec));
+  *this = Duration(duration_msg);
-  rcl_duration_.nanoseconds += duration_msg.nanosec;
   return *this;
 }
 
@@ -236,6 +234,10 @@ Duration::seconds() const
 rmw_time_t
 Duration::to_rmw_time() const
 {
+  if (rcl_duration_.nanoseconds < 0) {
+    throw std::runtime_error("rmw_time_t cannot be negative");
+  }
+
   // reuse conversion logic from msg creation
   builtin_interfaces::msg::Duration msg = *this;
   rmw_time_t result;

rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

@@ -159,7 +159,7 @@ __lockless_has_parameter(
 // see https://en.cppreference.com/w/cpp/types/numeric_limits/epsilon
 RCLCPP_LOCAL
 bool
-__are_doubles_equal(double x, double y, size_t ulp = 100)
+__are_doubles_equal(double x, double y, double ulp = 100.0)
 {
   return std::abs(x - y) <= std::numeric_limits<double>::epsilon() * std::abs(x + y) * ulp;
 }

rclcpp/src/rclcpp/node_options.cpp

@@ -338,7 +338,7 @@ NodeOptions::get_domain_id_from_env() const
   _dupenv_s(&ros_domain_id, &ros_domain_id_size, env_var);
 #endif
   if (ros_domain_id) {
-    uint32_t number = strtoul(ros_domain_id, NULL, 0);
+    uint32_t number = static_cast<uint32_t>(strtoul(ros_domain_id, NULL, 0));
     if (number == (std::numeric_limits<uint32_t>::max)()) {
 #ifdef _WIN32
       // free the ros_domain_id before throwing, if getenv was used on Windows

rclcpp/src/rclcpp/time.cpp

@@ -63,17 +63,13 @@ Time::Time(int64_t nanoseconds, rcl_clock_type_t clock_type)
   rcl_time_.nanoseconds = nanoseconds;
 }
 
-Time::Time(const Time & rhs)
+Time::Time(const Time & rhs) = default;
-: rcl_time_(rhs.rcl_time_)
-{
-  rcl_time_.nanoseconds = rhs.rcl_time_.nanoseconds;
-}
 
 Time::Time(
   const builtin_interfaces::msg::Time & time_msg,
-  rcl_clock_type_t ros_time)
+  rcl_clock_type_t clock_type)
+: rcl_time_(init_time_point(clock_type))
 {
-  rcl_time_ = init_time_point(ros_time);
   if (time_msg.sec < 0) {
     throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
   }
@@ -95,31 +91,25 @@ Time::~Time()
 Time::operator builtin_interfaces::msg::Time() const
 {
   builtin_interfaces::msg::Time msg_time;
-  msg_time.sec = static_cast<std::int32_t>(RCL_NS_TO_S(rcl_time_.nanoseconds));
+  constexpr rcl_time_point_value_t kRemainder = RCL_S_TO_NS(1);
-  msg_time.nanosec = static_cast<std::uint32_t>(rcl_time_.nanoseconds % (1000 * 1000 * 1000));
+  const auto result = std::div(rcl_time_.nanoseconds, kRemainder);
+  if (result.rem >= 0) {
+    msg_time.sec = static_cast<std::int32_t>(result.quot);
+    msg_time.nanosec = static_cast<std::uint32_t>(result.rem);
+  } else {
+    msg_time.sec = static_cast<std::int32_t>(result.quot - 1);
+    msg_time.nanosec = static_cast<std::uint32_t>(kRemainder + result.rem);
+  }
   return msg_time;
 }
 
 Time &
-Time::operator=(const Time & rhs)
+Time::operator=(const Time & rhs) = default;
-{
-  rcl_time_ = rhs.rcl_time_;
-  return *this;
-}
 
 Time &
 Time::operator=(const builtin_interfaces::msg::Time & time_msg)
 {
-  if (time_msg.sec < 0) {
+  *this = Time(time_msg);
-    throw std::runtime_error("cannot store a negative time point in rclcpp::Time");
-  }
-
-
-  rcl_clock_type_t ros_time = RCL_ROS_TIME;
-  rcl_time_ = init_time_point(ros_time);  // TODO(tfoote) hard coded ROS here
-
-  rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast<int64_t>(time_msg.sec));
-  rcl_time_.nanoseconds += time_msg.nanosec;
   return *this;
 }
 

rclcpp/test/rclcpp/executors/test_multi_threaded_executor.cpp

@@ -83,7 +83,7 @@ TEST_F(TestMultiThreadedExecutor, timer_over_take) {
 
       {
         std::lock_guard<std::mutex> lock(last_mutex);
-        double diff = std::abs((now - last).nanoseconds()) / 1.0e9;
+        double diff = static_cast<double>(std::abs((now - last).nanoseconds())) / 1.0e9;
         last = now;
 
         if (diff < PERIOD - TOLERANCE) {

rclcpp/test/rclcpp/test_duration.cpp

@@ -32,11 +32,7 @@ class TestDuration : public ::testing::Test
 {
 };
 
-// TEST(TestDuration, conversions) {
+TEST_F(TestDuration, operators) {
-// TODO(tfoote) Implement conversion methods
-// }
-
-TEST(TestDuration, operators) {
   rclcpp::Duration old(1, 0);
   rclcpp::Duration young(2, 0);
 
@@ -68,7 +64,7 @@ TEST(TestDuration, operators) {
   EXPECT_TRUE(time == assignment_op_duration);
 }
 
-TEST(TestDuration, chrono_overloads) {
+TEST_F(TestDuration, chrono_overloads) {
   int64_t ns = 123456789l;
   auto chrono_ns = std::chrono::nanoseconds(ns);
   auto d1 = rclcpp::Duration(ns);
@@ -87,7 +83,7 @@ TEST(TestDuration, chrono_overloads) {
   EXPECT_EQ(chrono_float_seconds, d5.to_chrono<decltype(chrono_float_seconds)>());
 }
 
-TEST(TestDuration, overflows) {
+TEST_F(TestDuration, overflows) {
   rclcpp::Duration max(std::numeric_limits<rcl_duration_value_t>::max());
   rclcpp::Duration min(std::numeric_limits<rcl_duration_value_t>::min());
 
@@ -108,7 +104,7 @@ TEST(TestDuration, overflows) {
   EXPECT_THROW(base_d_neg * 4, std::underflow_error);
 }
 
-TEST(TestDuration, negative_duration) {
+TEST_F(TestDuration, negative_duration) {
   rclcpp::Duration assignable_duration = rclcpp::Duration(0) - rclcpp::Duration(5, 0);
 
   {
@@ -131,7 +127,7 @@ TEST(TestDuration, negative_duration) {
   }
 }
 
-TEST(TestDuration, maximum_duration) {
+TEST_F(TestDuration, maximum_duration) {
   rclcpp::Duration max_duration = rclcpp::Duration::max();
   rclcpp::Duration max(std::numeric_limits<int32_t>::max(), 999999999);
 
@@ -139,18 +135,105 @@ TEST(TestDuration, maximum_duration) {
 }
 
 static const int64_t HALF_SEC_IN_NS = 500 * 1000 * 1000;
+static const int64_t ONE_SEC_IN_NS = 1000 * 1000 * 1000;
 static const int64_t ONE_AND_HALF_SEC_IN_NS = 3 * HALF_SEC_IN_NS;
 
-TEST(TestDuration, from_seconds) {
+TEST_F(TestDuration, from_seconds) {
   EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration::from_seconds(0.0));
   EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration::from_seconds(0));
   EXPECT_EQ(rclcpp::Duration(1, HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(1.5));
   EXPECT_EQ(rclcpp::Duration(-ONE_AND_HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(-1.5));
 }
 
-TEST(TestDuration, std_chrono_constructors) {
+TEST_F(TestDuration, std_chrono_constructors) {
   EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration(0.0s));
   EXPECT_EQ(rclcpp::Duration(0), rclcpp::Duration(0s));
   EXPECT_EQ(rclcpp::Duration(1, HALF_SEC_IN_NS), rclcpp::Duration(1.5s));
   EXPECT_EQ(rclcpp::Duration(-1, 0), rclcpp::Duration(-1s));
 }
+
+TEST_F(TestDuration, conversions) {
+  {
+    const rclcpp::Duration duration(HALF_SEC_IN_NS);
+    const auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
+    EXPECT_EQ(duration_msg.sec, 0);
+    EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
+    EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), HALF_SEC_IN_NS);
+
+    const auto rmw_time = duration.to_rmw_time();
+    EXPECT_EQ(rmw_time.sec, 0u);
+    EXPECT_EQ(rmw_time.nsec, static_cast<uint64_t>(HALF_SEC_IN_NS));
+
+    const auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
+    EXPECT_EQ(chrono_duration.count(), HALF_SEC_IN_NS);
+  }
+
+  {
+    const rclcpp::Duration duration(ONE_SEC_IN_NS);
+    const auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
+    EXPECT_EQ(duration_msg.sec, 1);
+    EXPECT_EQ(duration_msg.nanosec, 0u);
+    EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), ONE_SEC_IN_NS);
+
+    const auto rmw_time = duration.to_rmw_time();
+    EXPECT_EQ(rmw_time.sec, 1u);
+    EXPECT_EQ(rmw_time.nsec, 0u);
+
+    const auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
+    EXPECT_EQ(chrono_duration.count(), ONE_SEC_IN_NS);
+  }
+
+  {
+    const rclcpp::Duration duration(ONE_AND_HALF_SEC_IN_NS);
+    auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
+    EXPECT_EQ(duration_msg.sec, 1);
+    EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
+    EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), ONE_AND_HALF_SEC_IN_NS);
+
+    auto rmw_time = duration.to_rmw_time();
+    EXPECT_EQ(rmw_time.sec, 1u);
+    EXPECT_EQ(rmw_time.nsec, static_cast<uint64_t>(HALF_SEC_IN_NS));
+
+    auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
+    EXPECT_EQ(chrono_duration.count(), ONE_AND_HALF_SEC_IN_NS);
+  }
+
+  {
+    rclcpp::Duration duration(-HALF_SEC_IN_NS);
+    auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
+    EXPECT_EQ(duration_msg.sec, -1);
+    EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
+    EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), -HALF_SEC_IN_NS);
+
+    EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);
+
+    auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
+    EXPECT_EQ(chrono_duration.count(), -HALF_SEC_IN_NS);
+  }
+
+  {
+    rclcpp::Duration duration(-ONE_SEC_IN_NS);
+    auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
+    EXPECT_EQ(duration_msg.sec, -1);
+    EXPECT_EQ(duration_msg.nanosec, 0u);
+    EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), -ONE_SEC_IN_NS);
+
+    EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);
+
+    auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
+    EXPECT_EQ(chrono_duration.count(), -ONE_SEC_IN_NS);
+  }
+
+  {
+    rclcpp::Duration duration(-ONE_AND_HALF_SEC_IN_NS);
+    auto duration_msg = static_cast<builtin_interfaces::msg::Duration>(duration);
+    EXPECT_EQ(duration_msg.sec, -2);
+    EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS);
+    EXPECT_EQ(rclcpp::Duration(duration_msg).nanoseconds(), -ONE_AND_HALF_SEC_IN_NS);
+
+    EXPECT_THROW(duration.to_rmw_time(), std::runtime_error);
+
+    auto chrono_duration = duration.to_chrono<std::chrono::nanoseconds>();
+    EXPECT_EQ(chrono_duration.count(), -ONE_AND_HALF_SEC_IN_NS);
+  }
+}

rclcpp/test/rclcpp/test_parameter.cpp

@@ -32,7 +32,7 @@ protected:
   }
 };
 
-TEST(TestParameter, not_set_variant) {
+TEST_F(TestParameter, not_set_variant) {
   // Direct instantiation
   rclcpp::Parameter not_set_variant;
   EXPECT_EQ(rclcpp::PARAMETER_NOT_SET, not_set_variant.get_type());
@@ -58,7 +58,7 @@ TEST(TestParameter, not_set_variant) {
     rclcpp::Parameter::from_parameter_msg(not_set_param).get_type());
 }
 
-TEST(TestParameter, bool_variant) {
+TEST_F(TestParameter, bool_variant) {
   // Direct instantiation
   rclcpp::Parameter bool_variant_true("bool_param", true);
   EXPECT_EQ("bool_param", bool_variant_true.get_name());
@@ -116,7 +116,7 @@ TEST(TestParameter, bool_variant) {
     bool_variant_false.get_value_message().type);
 }
 
-TEST(TestParameter, integer_variant) {
+TEST_F(TestParameter, integer_variant) {
   const int TEST_VALUE {42};
 
   // Direct instantiation
@@ -164,7 +164,7 @@ TEST(TestParameter, integer_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, long_integer_variant) {
+TEST_F(TestParameter, long_integer_variant) {
   const int64_t TEST_VALUE {std::numeric_limits<int64_t>::max()};
 
   // Direct instantiation
@@ -212,7 +212,7 @@ TEST(TestParameter, long_integer_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, float_variant) {
+TEST_F(TestParameter, float_variant) {
   const float TEST_VALUE {42.0f};
 
   // Direct instantiation
@@ -260,7 +260,7 @@ TEST(TestParameter, float_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, double_variant) {
+TEST_F(TestParameter, double_variant) {
   const double TEST_VALUE {-42.1};
 
   // Direct instantiation
@@ -308,7 +308,7 @@ TEST(TestParameter, double_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, string_variant) {
+TEST_F(TestParameter, string_variant) {
   const std::string TEST_VALUE {"ROS2"};
 
   // Direct instantiation
@@ -354,7 +354,7 @@ TEST(TestParameter, string_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, byte_array_variant) {
+TEST_F(TestParameter, byte_array_variant) {
   const std::vector<uint8_t> TEST_VALUE {0x52, 0x4f, 0x53, 0x32};
 
   // Direct instantiation
@@ -402,7 +402,7 @@ TEST(TestParameter, byte_array_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, bool_array_variant) {
+TEST_F(TestParameter, bool_array_variant) {
   const std::vector<bool> TEST_VALUE {false, true, true, false, false, true};
 
   // Direct instantiation
@@ -450,7 +450,7 @@ TEST(TestParameter, bool_array_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, integer_array_variant) {
+TEST_F(TestParameter, integer_array_variant) {
   const std::vector<int> TEST_VALUE
   {42, -99, std::numeric_limits<int>::max(), std::numeric_limits<int>::lowest(), 0};
 
@@ -529,7 +529,7 @@ TEST(TestParameter, integer_array_variant) {
     rcl_interfaces::msg::ParameterType::PARAMETER_INTEGER_ARRAY);
 }
 
-TEST(TestParameter, long_integer_array_variant) {
+TEST_F(TestParameter, long_integer_array_variant) {
   const std::vector<int64_t> TEST_VALUE
   {42, -99, std::numeric_limits<int64_t>::max(), std::numeric_limits<int64_t>::lowest(), 0};
 
@@ -583,7 +583,7 @@ TEST(TestParameter, long_integer_array_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, float_array_variant) {
+TEST_F(TestParameter, float_array_variant) {
   const std::vector<float> TEST_VALUE
   {42.1f, -99.1f, std::numeric_limits<float>::max(), std::numeric_limits<float>::lowest(), 0.1f};
 
@@ -662,7 +662,7 @@ TEST(TestParameter, float_array_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, double_array_variant) {
+TEST_F(TestParameter, double_array_variant) {
   const std::vector<double> TEST_VALUE
   {42.1, -99.1, std::numeric_limits<double>::max(), std::numeric_limits<double>::lowest(), 0.1};
 
@@ -716,7 +716,7 @@ TEST(TestParameter, double_array_variant) {
     from_msg.get_value_message().type);
 }
 
-TEST(TestParameter, string_array_variant) {
+TEST_F(TestParameter, string_array_variant) {
   const std::vector<std::string> TEST_VALUE {"R", "O", "S2"};
 
   // Direct instantiation

rclcpp/test/rclcpp/test_time.cpp

@@ -45,7 +45,7 @@ protected:
   }
 };
 
-TEST(TestTime, clock_type_access) {
+TEST_F(TestTime, clock_type_access) {
   rclcpp::Clock ros_clock(RCL_ROS_TIME);
   EXPECT_EQ(RCL_ROS_TIME, ros_clock.get_clock_type());
 
@@ -57,7 +57,7 @@ TEST(TestTime, clock_type_access) {
 }
 
 // Check that the clock may go out of the scope before the jump callback without leading in UB.
-TEST(TestTime, clock_jump_callback_destruction_order) {
+TEST_F(TestTime, clock_jump_callback_destruction_order) {
   rclcpp::JumpHandler::SharedPtr handler;
   {
     rclcpp::Clock ros_clock(RCL_ROS_TIME);
@@ -68,7 +68,7 @@ TEST(TestTime, clock_jump_callback_destruction_order) {
   }
 }
 
-TEST(TestTime, time_sources) {
+TEST_F(TestTime, time_sources) {
   using builtin_interfaces::msg::Time;
   rclcpp::Clock ros_clock(RCL_ROS_TIME);
   Time ros_now = ros_clock.now();
@@ -86,25 +86,38 @@ TEST(TestTime, time_sources) {
   EXPECT_NE(0u, steady_now.nanosec);
 }
 
-TEST(TestTime, conversions) {
+static const int64_t HALF_SEC_IN_NS = 500 * 1000 * 1000;
+static const int64_t ONE_SEC_IN_NS = 1000 * 1000 * 1000;
+static const int64_t ONE_AND_HALF_SEC_IN_NS = 3 * HALF_SEC_IN_NS;
+
+TEST_F(TestTime, conversions) {
   rclcpp::Clock system_clock(RCL_SYSTEM_TIME);
 
+  {
     rclcpp::Time now = system_clock.now();
     builtin_interfaces::msg::Time now_msg = now;
 
     rclcpp::Time now_again = now_msg;
     EXPECT_EQ(now.nanoseconds(), now_again.nanoseconds());
+  }
 
-  builtin_interfaces::msg::Time msg;
+  {
-  msg.sec = 12345;
+    rclcpp::Time positive_time = rclcpp::Time(12345, 67890u);
-  msg.nanosec = 67890;
+
+    builtin_interfaces::msg::Time msg = positive_time;
+    EXPECT_EQ(msg.sec, 12345);
+    EXPECT_EQ(msg.nanosec, 67890u);
 
     rclcpp::Time time = msg;
+    EXPECT_EQ(time.nanoseconds(), positive_time.nanoseconds());
     EXPECT_EQ(
       RCL_S_TO_NS(static_cast<int64_t>(msg.sec)) + static_cast<int64_t>(msg.nanosec),
       time.nanoseconds());
     EXPECT_EQ(static_cast<int64_t>(msg.sec), RCL_NS_TO_S(time.nanoseconds()));
+  }
 
+  // throw on construction/assignment of negative times
+  {
     builtin_interfaces::msg::Time negative_time_msg;
     negative_time_msg.sec = -1;
     negative_time_msg.nanosec = 1;
@@ -121,9 +134,76 @@ TEST(TestTime, conversions) {
       rclcpp::Time assignment(1, 2);
       assignment = negative_time_msg;
     });
+  }
+
+  {
+    const rclcpp::Time time(HALF_SEC_IN_NS);
+    const auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, 0);
+    EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
+    EXPECT_EQ(rclcpp::Time(time_msg).nanoseconds(), HALF_SEC_IN_NS);
+  }
+
+  {
+    const rclcpp::Time time(ONE_SEC_IN_NS);
+    const auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, 1);
+    EXPECT_EQ(time_msg.nanosec, 0u);
+    EXPECT_EQ(rclcpp::Time(time_msg).nanoseconds(), ONE_SEC_IN_NS);
+  }
+
+  {
+    const rclcpp::Time time(ONE_AND_HALF_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, 1);
+    EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
+    EXPECT_EQ(rclcpp::Time(time_msg).nanoseconds(), ONE_AND_HALF_SEC_IN_NS);
+  }
+
+  {
+    // Can rclcpp::Time be negative or not? The following constructor works:
+    rclcpp::Time time(-HALF_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, -1);
+    EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
+
+    // The opposite conversion throws...
+    EXPECT_ANY_THROW(
+    {
+      rclcpp::Time negative_time(time_msg);
+    });
+  }
+
+  {
+    // Can rclcpp::Time be negative or not? The following constructor works:
+    rclcpp::Time time(-ONE_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, -1);
+    EXPECT_EQ(time_msg.nanosec, 0u);
+
+    // The opposite conversion throws...
+    EXPECT_ANY_THROW(
+    {
+      rclcpp::Time negative_time(time_msg);
+    });
+  }
+
+  {
+    // Can rclcpp::Time be negative or not? The following constructor works:
+    rclcpp::Time time(-ONE_AND_HALF_SEC_IN_NS);
+    auto time_msg = static_cast<builtin_interfaces::msg::Time>(time);
+    EXPECT_EQ(time_msg.sec, -2);
+    EXPECT_EQ(time_msg.nanosec, HALF_SEC_IN_NS);
+
+    // The opposite conversion throws...
+    EXPECT_ANY_THROW(
+    {
+      rclcpp::Time negative_time(time_msg);
+    });
+  }
 }
 
-TEST(TestTime, operators) {
+TEST_F(TestTime, operators) {
   rclcpp::Time old(1, 0);
   rclcpp::Time young(2, 0);
 
@@ -178,7 +258,7 @@ TEST(TestTime, operators) {
   }
 }
 
-TEST(TestTime, overflow_detectors) {
+TEST_F(TestTime, overflow_detectors) {
   /////////////////////////////////////////////////////////////////////////////
   // Test logical_eq call first:
   EXPECT_TRUE(logical_eq(false, false));
@@ -198,8 +278,8 @@ TEST(TestTime, overflow_detectors) {
   // 256 * 256 = 64K total loops, should be pretty fast on everything
   for (big_type_t y = min_val; y <= max_val; ++y) {
     for (big_type_t x = min_val; x <= max_val; ++x) {
-      const big_type_t sum = x + y;
+      const big_type_t sum = static_cast<big_type_t>(x + y);
-      const big_type_t diff = x - y;
+      const big_type_t diff = static_cast<big_type_t>(x - y);
 
       const bool add_will_overflow =
         rclcpp::add_will_overflow(test_type_t(x), test_type_t(y));
@@ -235,7 +315,7 @@ TEST(TestTime, overflow_detectors) {
   EXPECT_TRUE(rclcpp::sub_will_underflow<int64_t>(INT64_MIN, 1));
 }
 
-TEST(TestTime, overflows) {
+TEST_F(TestTime, overflows) {
   rclcpp::Time max_time(std::numeric_limits<rcl_time_point_value_t>::max());
   rclcpp::Time min_time(std::numeric_limits<rcl_time_point_value_t>::min());
   rclcpp::Duration one(1);
@@ -267,7 +347,7 @@ TEST(TestTime, overflows) {
   EXPECT_NO_THROW(one_time - two_time);
 }
 
-TEST(TestTime, seconds) {
+TEST_F(TestTime, seconds) {
   EXPECT_DOUBLE_EQ(0.0, rclcpp::Time(0, 0).seconds());
   EXPECT_DOUBLE_EQ(4.5, rclcpp::Time(4, 500000000).seconds());
   EXPECT_DOUBLE_EQ(2.5, rclcpp::Time(0, 2500000000).seconds());