From 3a503685bf83280ffcde3ecb2d33a7d8de7dade2 Mon Sep 17 00:00:00 2001 From: serge-nikulin Date: Thu, 1 Feb 2018 13:50:33 -0800 Subject: [PATCH] change rcutils_time_point_value_t type from uint64_t to int64_t (#429) * change rcutils_time_point_value_t type from uint64_t to int64_t * small style changes * fix test time datatype * Update time primatives to int64_t * change time primitive datatype to signed * A few more instances of UL to L --- rclcpp/include/rclcpp/time.hpp | 2 +- rclcpp/include/rclcpp/utilities.hpp | 69 ++++++++++++++++++++++ rclcpp/src/rclcpp/time.cpp | 56 ++++++++---------- rclcpp/test/test_node.cpp | 2 +- rclcpp/test/test_time.cpp | 92 ++++++++++++++++++++++++++++- rclcpp/test/test_time_source.cpp | 10 ++-- 6 files changed, 191 insertions(+), 40 deletions(-) diff --git a/rclcpp/include/rclcpp/time.hpp b/rclcpp/include/rclcpp/time.hpp index f6c8df8721..22e0335da8 100644 --- a/rclcpp/include/rclcpp/time.hpp +++ b/rclcpp/include/rclcpp/time.hpp @@ -35,7 +35,7 @@ class Time Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type = RCL_SYSTEM_TIME); RCLCPP_PUBLIC - explicit Time(uint64_t nanoseconds = 0, rcl_clock_type_t clock = RCL_SYSTEM_TIME); + explicit Time(int64_t nanoseconds = 0, rcl_clock_type_t clock = RCL_SYSTEM_TIME); RCLCPP_PUBLIC Time(const Time & rhs); diff --git a/rclcpp/include/rclcpp/utilities.hpp b/rclcpp/include/rclcpp/utilities.hpp index a17a631257..987d1347ec 100644 --- a/rclcpp/include/rclcpp/utilities.hpp +++ b/rclcpp/include/rclcpp/utilities.hpp @@ -17,6 +17,7 @@ #include #include +#include #include "rclcpp/visibility_control.hpp" @@ -109,6 +110,74 @@ RCLCPP_PUBLIC bool sleep_for(const std::chrono::nanoseconds & nanoseconds); +/// Safely check if addition will overflow. +/** + * The type of the operands, T, should have defined + * std::numeric_limits::max(), `>`, `<` and `-` operators. + * + * \param[in] x is the first addend. + * \param[in] y is the second addend. + * \tparam T is type of the operands. + * \return True if the x + y sum is greater than T::max value. + */ +template +bool +add_will_overflow(const T x, const T y) +{ + return (y > 0) && (x > (std::numeric_limits::max() - y)); +} + +/// Safely check if addition will underflow. +/** + * The type of the operands, T, should have defined + * std::numeric_limits::min(), `>`, `<` and `-` operators. + * + * \param[in] x is the first addend. + * \param[in] y is the second addend. + * \tparam T is type of the operands. + * \return True if the x + y sum is less than T::min value. + */ +template +bool +add_will_underflow(const T x, const T y) +{ + return (y < 0) && (x < (std::numeric_limits::min() - y)); +} + +/// Safely check if subtraction will overflow. +/** + * The type of the operands, T, should have defined + * std::numeric_limits::max(), `>`, `<` and `+` operators. + * + * \param[in] x is the minuend. + * \param[in] y is the subtrahend. + * \tparam T is type of the operands. + * \return True if the difference `x - y` sum is grater than T::max value. + */ +template +bool +sub_will_overflow(const T x, const T y) +{ + return (y < 0) && (x > (std::numeric_limits::max() + y)); +} + +/// Safely check if subtraction will underflow. +/** + * The type of the operands, T, should have defined + * std::numeric_limits::min(), `>`, `<` and `+` operators. + * + * \param[in] x is the minuend. + * \param[in] y is the subtrahend. + * \tparam T is type of the operands. + * \return True if the difference `x - y` sum is less than T::min value. + */ +template +bool +sub_will_underflow(const T x, const T y) +{ + return (y > 0) && (x < (std::numeric_limits::min() + y)); +} + } // namespace rclcpp #endif // RCLCPP__UTILITIES_HPP_ diff --git a/rclcpp/src/rclcpp/time.cpp b/rclcpp/src/rclcpp/time.cpp index ed3afa22af..c22ec50127 100644 --- a/rclcpp/src/rclcpp/time.cpp +++ b/rclcpp/src/rclcpp/time.cpp @@ -26,6 +26,8 @@ #include "rcutils/logging_macros.h" +#include "rclcpp/utilities.hpp" + namespace { @@ -50,11 +52,11 @@ Time::Time(int32_t seconds, uint32_t nanoseconds, rcl_clock_type_t clock_type) throw std::runtime_error("cannot store a negative time point in rclcpp::Time"); } - rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast(seconds)); + rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast(seconds)); rcl_time_.nanoseconds += nanoseconds; } -Time::Time(uint64_t nanoseconds, rcl_clock_type_t clock_type) +Time::Time(int64_t nanoseconds, rcl_clock_type_t clock_type) : rcl_time_(init_time_point(clock_type)) { rcl_time_.nanoseconds = nanoseconds; @@ -75,7 +77,7 @@ Time::Time( throw std::runtime_error("cannot store a negative time point in rclcpp::Time"); } - rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast(time_msg.sec)); + rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast(time_msg.sec)); rcl_time_.nanoseconds += time_msg.nanosec; } @@ -115,7 +117,7 @@ Time::operator=(const builtin_interfaces::msg::Time & time_msg) 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(time_msg.sec)); + rcl_time_.nanoseconds = RCL_S_TO_NS(static_cast(time_msg.sec)); rcl_time_.nanoseconds += time_msg.nanosec; return *this; } @@ -179,11 +181,11 @@ Time::operator>(const rclcpp::Time & rhs) const Time Time::operator+(const rclcpp::Duration & rhs) const { - if (rhs.nanoseconds() > 0 && (uint64_t)rhs.nanoseconds() > - std::numeric_limits::max() - - (rcl_time_point_value_t)this->nanoseconds()) - { - throw std::overflow_error("addition leads to uint64_t overflow"); + if (rclcpp::add_will_overflow(rhs.nanoseconds(), this->nanoseconds())) { + throw std::overflow_error("addition leads to int64_t overflow"); + } + if (rclcpp::add_will_underflow(rhs.nanoseconds(), this->nanoseconds())) { + throw std::underflow_error("addition leads to int64_t underflow"); } return Time(this->nanoseconds() + rhs.nanoseconds(), this->get_clock_type()); } @@ -195,17 +197,12 @@ Time::operator-(const rclcpp::Time & rhs) const throw std::runtime_error("can't subtract times with different time sources"); } - if (rcl_time_.nanoseconds > - (uint64_t)std::numeric_limits::max() + rhs.rcl_time_.nanoseconds) - { - throw std::underflow_error("time subtraction leads to int64_t overflow"); + if (rclcpp::sub_will_overflow(rcl_time_.nanoseconds, rhs.rcl_time_.nanoseconds)) { + throw std::overflow_error("time subtraction leads to int64_t overflow"); } - if (rcl_time_.nanoseconds < rhs.rcl_time_.nanoseconds) { - rcl_time_point_value_t negative_delta = rhs.rcl_time_.nanoseconds - rcl_time_.nanoseconds; - if (negative_delta > (uint64_t) std::numeric_limits::min()) { - throw std::underflow_error("time subtraction leads to int64_t underflow"); - } + if (rclcpp::sub_will_underflow(rcl_time_.nanoseconds, rhs.rcl_time_.nanoseconds)) { + throw std::underflow_error("time subtraction leads to int64_t underflow"); } return Duration(rcl_time_.nanoseconds - rhs.rcl_time_.nanoseconds); @@ -214,21 +211,17 @@ Time::operator-(const rclcpp::Time & rhs) const Time Time::operator-(const rclcpp::Duration & rhs) const { - if (rhs.nanoseconds() > 0 && rcl_time_.nanoseconds > - std::numeric_limits::max() - (uint64_t)rhs.nanoseconds()) - { - throw std::underflow_error("time subtraction leads to uint64_t overflow"); + if (rclcpp::sub_will_overflow(rcl_time_.nanoseconds, rhs.nanoseconds())) { + throw std::overflow_error("time subtraction leads to int64_t overflow"); } - if (rcl_time_.nanoseconds < (uint64_t) std::numeric_limits::max() && - (int64_t)rcl_time_.nanoseconds < (int64_t)rhs.nanoseconds()) - { - throw std::underflow_error("time subtraction leads to uint64_t underflow"); + if (rclcpp::sub_will_underflow(rcl_time_.nanoseconds, rhs.nanoseconds())) { + throw std::underflow_error("time subtraction leads to int64_t underflow"); } return Time(rcl_time_.nanoseconds - rhs.nanoseconds(), rcl_time_.clock_type); } -uint64_t +int64_t Time::nanoseconds() const { return rcl_time_.nanoseconds; @@ -243,10 +236,11 @@ Time::get_clock_type() const Time operator+(const rclcpp::Duration & lhs, const rclcpp::Time & rhs) { - if (rhs.nanoseconds() > - std::numeric_limits::max() - (rcl_time_point_value_t)lhs.nanoseconds()) - { - throw std::overflow_error("addition leads to uint64_t overflow"); + if (rclcpp::add_will_overflow(rhs.nanoseconds(), lhs.nanoseconds())) { + throw std::overflow_error("addition leads to int64_t overflow"); + } + if (rclcpp::add_will_underflow(rhs.nanoseconds(), lhs.nanoseconds())) { + throw std::underflow_error("addition leads to int64_t underflow"); } return Time(lhs.nanoseconds() + rhs.nanoseconds(), rhs.get_clock_type()); } diff --git a/rclcpp/test/test_node.cpp b/rclcpp/test/test_node.cpp index 8ee9aa77f6..2346250538 100644 --- a/rclcpp/test/test_node.cpp +++ b/rclcpp/test/test_node.cpp @@ -99,5 +99,5 @@ TEST_F(TestNode, now) { auto now_builtin = node->now().nanoseconds(); auto now_external = clock->now().nanoseconds(); EXPECT_GE(now_external, now_builtin); - EXPECT_LT(now_external - now_builtin, 50000ul); + EXPECT_LT(now_external - now_builtin, 50000L); } diff --git a/rclcpp/test/test_time.cpp b/rclcpp/test/test_time.cpp index 448d123e70..69e37789b6 100644 --- a/rclcpp/test/test_time.cpp +++ b/rclcpp/test/test_time.cpp @@ -23,6 +23,18 @@ #include "rclcpp/clock.hpp" #include "rclcpp/rclcpp.hpp" #include "rclcpp/time.hpp" +#include "rclcpp/utilities.hpp" + +namespace +{ + +bool logical_eq(const bool a, const bool b) +{ + return (a && b) || ((!a) && !(b)); +} + +} // namespace + class TestTime : public ::testing::Test { @@ -77,9 +89,9 @@ TEST(TestTime, conversions) { rclcpp::Time time = msg; EXPECT_EQ( - RCL_S_TO_NS(static_cast(msg.sec)) + static_cast(msg.nanosec), + RCL_S_TO_NS(static_cast(msg.sec)) + static_cast(msg.nanosec), time.nanoseconds()); - EXPECT_EQ(static_cast(msg.sec), RCL_NS_TO_S(time.nanoseconds())); + EXPECT_EQ(static_cast(msg.sec), RCL_NS_TO_S(time.nanoseconds())); builtin_interfaces::msg::Time negative_time_msg; negative_time_msg.sec = -1; @@ -148,11 +160,87 @@ TEST(TestTime, operators) { } } +TEST(TestTime, overflow_detectors) { + ///////////////////////////////////////////////////////////////////////////// + // Test logical_eq call first: + EXPECT_TRUE(logical_eq(false, false)); + EXPECT_FALSE(logical_eq(false, true)); + EXPECT_FALSE(logical_eq(true, false)); + EXPECT_TRUE(logical_eq(true, true)); + + ///////////////////////////////////////////////////////////////////////////// + // Exhaustive test of all int8_t values + using test_type_t = int8_t; + // big_type_t encompasses test_type_t: + // big_type_t::min < test_type_t::min + // big_type_t::max > test_type_t::max + using big_type_t = int16_t; + const big_type_t min_val = std::numeric_limits::min(); + const big_type_t max_val = std::numeric_limits::max(); + // 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 diff = x - y; + + const bool add_will_overflow = + rclcpp::add_will_overflow(test_type_t(x), test_type_t(y)); + const bool add_did_overflow = sum > max_val; + EXPECT_TRUE(logical_eq(add_will_overflow, add_did_overflow)); + + const bool add_will_underflow = + rclcpp::add_will_underflow(test_type_t(x), test_type_t(y)); + const bool add_did_underflow = sum < min_val; + EXPECT_TRUE(logical_eq(add_will_underflow, add_did_underflow)); + + const bool sub_will_overflow = + rclcpp::sub_will_overflow(test_type_t(x), test_type_t(y)); + const bool sub_did_overflow = diff > max_val; + EXPECT_TRUE(logical_eq(sub_will_overflow, sub_did_overflow)); + + const bool sub_will_underflow = + rclcpp::sub_will_underflow(test_type_t(x), test_type_t(y)); + const bool sub_did_underflow = diff < min_val; + EXPECT_TRUE(logical_eq(sub_will_underflow, sub_did_underflow)); + } + } + + // Few selected tests for int64_t + EXPECT_TRUE(rclcpp::add_will_overflow(INT64_MAX, 1)); + EXPECT_FALSE(rclcpp::add_will_overflow(INT64_MAX, -1)); + EXPECT_TRUE(rclcpp::add_will_underflow(INT64_MIN, -1)); + EXPECT_FALSE(rclcpp::add_will_underflow(INT64_MIN, 1)); + + EXPECT_FALSE(rclcpp::sub_will_overflow(INT64_MAX, 1)); + EXPECT_TRUE(rclcpp::sub_will_overflow(INT64_MAX, -1)); + EXPECT_FALSE(rclcpp::sub_will_underflow(INT64_MIN, -1)); + EXPECT_TRUE(rclcpp::sub_will_underflow(INT64_MIN, 1)); +} + TEST(TestTime, overflows) { rclcpp::Time max_time(std::numeric_limits::max()); rclcpp::Time min_time(std::numeric_limits::min()); rclcpp::Duration one(1); + rclcpp::Duration two(2); + // Cross min/max EXPECT_THROW(max_time + one, std::overflow_error); EXPECT_THROW(min_time - one, std::underflow_error); + EXPECT_THROW(max_time - min_time, std::overflow_error); + EXPECT_THROW(min_time - max_time, std::underflow_error); + EXPECT_NO_THROW(max_time - max_time); + EXPECT_NO_THROW(min_time - min_time); + + // Cross zero in both directions + rclcpp::Time one_time(1); + EXPECT_NO_THROW(one_time - two); + + rclcpp::Time minus_one_time(-1); + EXPECT_NO_THROW(minus_one_time + two); + + EXPECT_NO_THROW(one_time - minus_one_time); + EXPECT_NO_THROW(minus_one_time - one_time); + + rclcpp::Time two_time(2); + EXPECT_NO_THROW(one_time - two_time); } diff --git a/rclcpp/test/test_time_source.cpp b/rclcpp/test/test_time_source.cpp index 5443f66bfb..a6f620ab7b 100644 --- a/rclcpp/test/test_time_source.cpp +++ b/rclcpp/test/test_time_source.cpp @@ -116,7 +116,7 @@ TEST_F(TestTimeSource, clock) { auto t_out = ros_clock->now(); - EXPECT_NE(0UL, t_out.nanoseconds()); + EXPECT_NE(0L, t_out.nanoseconds()); EXPECT_LT(t_low.nanoseconds(), t_out.nanoseconds()); EXPECT_GT(t_high.nanoseconds(), t_out.nanoseconds()); } @@ -190,7 +190,7 @@ TEST_F(TestTimeSource, callbacks) { auto t_out = ros_clock->now(); - EXPECT_NE(0UL, t_out.nanoseconds()); + EXPECT_NE(0L, t_out.nanoseconds()); EXPECT_LT(t_low.nanoseconds(), t_out.nanoseconds()); EXPECT_GT(t_high.nanoseconds(), t_out.nanoseconds()); @@ -222,7 +222,7 @@ TEST_F(TestTimeSource, callbacks) { t_out = ros_clock->now(); - EXPECT_NE(0UL, t_out.nanoseconds()); + EXPECT_NE(0L, t_out.nanoseconds()); EXPECT_LT(t_low.nanoseconds(), t_out.nanoseconds()); EXPECT_GT(t_high.nanoseconds(), t_out.nanoseconds()); } @@ -296,7 +296,7 @@ TEST_F(TestTimeSource, callback_handler_erasure) { auto t_out = ros_clock->now(); - EXPECT_NE(0UL, t_out.nanoseconds()); + EXPECT_NE(0L, t_out.nanoseconds()); EXPECT_LT(t_low.nanoseconds(), t_out.nanoseconds()); EXPECT_GT(t_high.nanoseconds(), t_out.nanoseconds()); @@ -320,7 +320,7 @@ TEST_F(TestTimeSource, callback_handler_erasure) { t_out = ros_clock->now(); - EXPECT_NE(0UL, t_out.nanoseconds()); + EXPECT_NE(0L, t_out.nanoseconds()); EXPECT_LT(t_low.nanoseconds(), t_out.nanoseconds()); EXPECT_GT(t_high.nanoseconds(), t_out.nanoseconds()); }