From 64f1a3e4a3585023b8959a92bb3a58bbc4640244 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 30 Oct 2020 16:19:55 -0300 Subject: [PATCH 1/6] Deprecate Duration(rcl_duration_value_t) in favor of static Duration::from_nanoseconds(rcl_duration_value_t) Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/duration.hpp | 11 ++++++-- rclcpp/src/rclcpp/duration.cpp | 20 ++++++++++---- rclcpp/src/rclcpp/time.cpp | 2 +- rclcpp/test/rclcpp/test_duration.cpp | 40 +++++++++++++++------------- rclcpp/test/rclcpp/test_time.cpp | 31 +++++++++++---------- 5 files changed, 63 insertions(+), 41 deletions(-) diff --git a/rclcpp/include/rclcpp/duration.hpp b/rclcpp/include/rclcpp/duration.hpp index 49125ec722..b350821a10 100644 --- a/rclcpp/include/rclcpp/duration.hpp +++ b/rclcpp/include/rclcpp/duration.hpp @@ -38,10 +38,11 @@ class RCLCPP_PUBLIC Duration */ Duration(int32_t seconds, uint32_t nanoseconds); - // This constructor matches any numeric value - ints or floats. + /// Construct duration from the specified nanoseconds. + [[deprecated("Use Duration::from_nanoseconds instead")]] explicit Duration(rcl_duration_value_t nanoseconds); - // This constructor matches std::chrono::nanoseconds. + /// Construct duration from the specified std::chrono::nanoseconds. explicit Duration(std::chrono::nanoseconds nanoseconds); // This constructor matches any std::chrono value other than nanoseconds @@ -129,6 +130,10 @@ class RCLCPP_PUBLIC Duration static Duration from_seconds(double seconds); + /// Create a duration object from an integer number representing nanoseconds + static Duration + from_nanoseconds(rcl_duration_value_t nanoseconds); + /// Convert Duration into a std::chrono::Duration. template DurationT @@ -143,6 +148,8 @@ class RCLCPP_PUBLIC Duration private: rcl_duration_t rcl_duration_; + + Duration() = default; }; } // namespace rclcpp diff --git a/rclcpp/src/rclcpp/duration.cpp b/rclcpp/src/rclcpp/duration.cpp index 47faa0d611..383ad6499e 100644 --- a/rclcpp/src/rclcpp/duration.cpp +++ b/rclcpp/src/rclcpp/duration.cpp @@ -37,7 +37,7 @@ Duration::Duration(int32_t seconds, uint32_t nanoseconds) rcl_duration_.nanoseconds += nanoseconds; } -Duration::Duration(int64_t nanoseconds) +Duration::Duration(rcl_duration_value_t nanoseconds) { rcl_duration_.nanoseconds = nanoseconds; } @@ -148,7 +148,7 @@ Duration::operator+(const rclcpp::Duration & rhs) const this->rcl_duration_.nanoseconds, rhs.rcl_duration_.nanoseconds, std::numeric_limits::max()); - return Duration( + return Duration::from_nanoseconds( rcl_duration_.nanoseconds + rhs.rcl_duration_.nanoseconds); } @@ -177,7 +177,7 @@ Duration::operator-(const rclcpp::Duration & rhs) const rhs.rcl_duration_.nanoseconds, std::numeric_limits::max()); - return Duration( + return Duration::from_nanoseconds( rcl_duration_.nanoseconds - rhs.rcl_duration_.nanoseconds); } @@ -208,7 +208,7 @@ Duration::operator*(double scale) const scale, std::numeric_limits::max()); long double scale_ld = static_cast(scale); - return Duration( + return Duration::from_nanoseconds( static_cast( static_cast(rcl_duration_.nanoseconds) * scale_ld)); } @@ -249,7 +249,17 @@ Duration::to_rmw_time() const Duration Duration::from_seconds(double seconds) { - return Duration(static_cast(RCL_S_TO_NS(seconds))); + Duration ret; + ret.rcl_duration_.nanoseconds = static_cast(RCL_S_TO_NS(seconds)); + return ret; +} + +Duration +Duration::from_nanoseconds(rcl_duration_value_t seconds) +{ + Duration ret; + ret.rcl_duration_.nanoseconds = seconds; + return ret; } } // namespace rclcpp diff --git a/rclcpp/src/rclcpp/time.cpp b/rclcpp/src/rclcpp/time.cpp index e9d633e046..556a5e69ad 100644 --- a/rclcpp/src/rclcpp/time.cpp +++ b/rclcpp/src/rclcpp/time.cpp @@ -199,7 +199,7 @@ Time::operator-(const rclcpp::Time & rhs) const throw std::underflow_error("time subtraction leads to int64_t underflow"); } - return Duration(rcl_time_.nanoseconds - rhs.rcl_time_.nanoseconds); + return Duration::from_nanoseconds(rcl_time_.nanoseconds - rhs.rcl_time_.nanoseconds); } Time diff --git a/rclcpp/test/rclcpp/test_duration.cpp b/rclcpp/test/rclcpp/test_duration.cpp index 6e93ae019d..1c608faa61 100644 --- a/rclcpp/test/rclcpp/test_duration.cpp +++ b/rclcpp/test/rclcpp/test_duration.cpp @@ -68,7 +68,7 @@ TEST_F(TestDuration, operators) { TEST_F(TestDuration, chrono_overloads) { int64_t ns = 123456789l; auto chrono_ns = std::chrono::nanoseconds(ns); - auto d1 = rclcpp::Duration(ns); + auto d1 = rclcpp::Duration::from_nanoseconds(ns); auto d2 = rclcpp::Duration(chrono_ns); auto d3 = rclcpp::Duration(123456789ns); EXPECT_EQ(d1, d2); @@ -85,11 +85,11 @@ TEST_F(TestDuration, chrono_overloads) { } TEST_F(TestDuration, overflows) { - rclcpp::Duration max(std::numeric_limits::max()); - rclcpp::Duration min(std::numeric_limits::min()); + auto max = rclcpp::Duration::from_nanoseconds(std::numeric_limits::max()); + auto min = rclcpp::Duration::from_nanoseconds(std::numeric_limits::min()); - rclcpp::Duration one(1); - rclcpp::Duration negative_one(-1); + rclcpp::Duration one(1ns); + rclcpp::Duration negative_one(-1ns); EXPECT_THROW(max + one, std::overflow_error); EXPECT_THROW(min - one, std::underflow_error); @@ -106,7 +106,7 @@ TEST_F(TestDuration, overflows) { } TEST_F(TestDuration, negative_duration) { - rclcpp::Duration assignable_duration = rclcpp::Duration(0) - rclcpp::Duration(5, 0); + rclcpp::Duration assignable_duration = rclcpp::Duration(0ns) - rclcpp::Duration(5, 0); { // avoid windows converting a literal number less than -INT_MAX to unsigned int C4146 @@ -140,22 +140,23 @@ 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(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(0ns), rclcpp::Duration::from_seconds(0.0)); + EXPECT_EQ(rclcpp::Duration(0ns), 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)); + EXPECT_EQ(rclcpp::Duration::from_nanoseconds( + -ONE_AND_HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(-1.5)); } 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(0ns), rclcpp::Duration(0.0s)); + EXPECT_EQ(rclcpp::Duration(0ns), 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); + auto duration = rclcpp::Duration::from_nanoseconds(HALF_SEC_IN_NS); const auto duration_msg = static_cast(duration); EXPECT_EQ(duration_msg.sec, 0); EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS); @@ -170,7 +171,7 @@ TEST_F(TestDuration, conversions) { } { - const rclcpp::Duration duration(ONE_SEC_IN_NS); + auto duration = rclcpp::Duration::from_nanoseconds(ONE_SEC_IN_NS); const auto duration_msg = static_cast(duration); EXPECT_EQ(duration_msg.sec, 1); EXPECT_EQ(duration_msg.nanosec, 0u); @@ -185,7 +186,7 @@ TEST_F(TestDuration, conversions) { } { - const rclcpp::Duration duration(ONE_AND_HALF_SEC_IN_NS); + auto duration = rclcpp::Duration::from_nanoseconds(ONE_AND_HALF_SEC_IN_NS); auto duration_msg = static_cast(duration); EXPECT_EQ(duration_msg.sec, 1); EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS); @@ -200,7 +201,7 @@ TEST_F(TestDuration, conversions) { } { - rclcpp::Duration duration(-HALF_SEC_IN_NS); + auto duration = rclcpp::Duration::from_nanoseconds(-HALF_SEC_IN_NS); auto duration_msg = static_cast(duration); EXPECT_EQ(duration_msg.sec, -1); EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS); @@ -213,7 +214,7 @@ TEST_F(TestDuration, conversions) { } { - rclcpp::Duration duration(-ONE_SEC_IN_NS); + auto duration = rclcpp::Duration::from_nanoseconds(-ONE_SEC_IN_NS); auto duration_msg = static_cast(duration); EXPECT_EQ(duration_msg.sec, -1); EXPECT_EQ(duration_msg.nanosec, 0u); @@ -226,7 +227,7 @@ TEST_F(TestDuration, conversions) { } { - rclcpp::Duration duration(-ONE_AND_HALF_SEC_IN_NS); + auto duration = rclcpp::Duration::from_nanoseconds(-ONE_AND_HALF_SEC_IN_NS); auto duration_msg = static_cast(duration); EXPECT_EQ(duration_msg.sec, -2); EXPECT_EQ(duration_msg.nanosec, HALF_SEC_IN_NS); @@ -253,9 +254,10 @@ TEST_F(TestDuration, test_some_constructors) { } TEST_F(TestDuration, test_some_exceptions) { - rclcpp::Duration test_duration(0u); + rclcpp::Duration test_duration(0ns); RCLCPP_EXPECT_THROW_EQ( - test_duration = rclcpp::Duration(INT64_MAX) - rclcpp::Duration(-1), + test_duration = + rclcpp::Duration::from_nanoseconds(INT64_MAX) - rclcpp::Duration(-1ns), std::overflow_error("duration subtraction leads to int64_t overflow")); RCLCPP_EXPECT_THROW_EQ( test_duration = test_duration * (std::numeric_limits::infinity()), diff --git a/rclcpp/test/rclcpp/test_time.cpp b/rclcpp/test/rclcpp/test_time.cpp index c399e51074..f7889e96f6 100644 --- a/rclcpp/test/rclcpp/test_time.cpp +++ b/rclcpp/test/rclcpp/test_time.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include @@ -30,6 +31,8 @@ namespace { +using namespace std::chrono_literals; + bool logical_eq(const bool a, const bool b) { return (a && b) || ((!a) && !(b)); @@ -221,7 +224,7 @@ TEST_F(TestTime, operators) { EXPECT_EQ(sub, young - old); rclcpp::Time young_changed(young); - young_changed -= rclcpp::Duration(old.nanoseconds()); + young_changed -= rclcpp::Duration::from_nanoseconds(old.nanoseconds()); EXPECT_EQ(sub.nanoseconds(), young_changed.nanoseconds()); rclcpp::Time system_time(0, 0, RCL_SYSTEM_TIME); @@ -320,8 +323,8 @@ TEST_F(TestTime, overflow_detectors) { TEST_F(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); + rclcpp::Duration one(1ns); + rclcpp::Duration two(2ns); // Cross min/max EXPECT_THROW(max_time + one, std::overflow_error); @@ -394,7 +397,7 @@ TEST_F(TestTime, test_assignment_operator_from_builtin_msg_time) { } TEST_F(TestTime, test_sum_operator) { - const rclcpp::Duration one(1); + const rclcpp::Duration one(1ns); const rclcpp::Time test_time(0u); EXPECT_EQ(0u, test_time.nanoseconds()); @@ -406,41 +409,41 @@ TEST_F(TestTime, test_overflow_underflow_throws) { rclcpp::Time test_time(0u); RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Time(INT64_MAX) + rclcpp::Duration(1), + test_time = rclcpp::Time(INT64_MAX) + rclcpp::Duration(1ns), std::overflow_error("addition leads to int64_t overflow")); RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Time(INT64_MIN) + rclcpp::Duration(-1), + test_time = rclcpp::Time(INT64_MIN) + rclcpp::Duration(-1ns), std::underflow_error("addition leads to int64_t underflow")); RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Time(INT64_MAX) - rclcpp::Duration(-1), + test_time = rclcpp::Time(INT64_MAX) - rclcpp::Duration(-1ns), std::overflow_error("time subtraction leads to int64_t overflow")); RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Time(INT64_MIN) - rclcpp::Duration(1), + test_time = rclcpp::Time(INT64_MIN) - rclcpp::Duration(1ns), std::underflow_error("time subtraction leads to int64_t underflow")); test_time = rclcpp::Time(INT64_MAX); RCLCPP_EXPECT_THROW_EQ( - test_time += rclcpp::Duration(1), + test_time += rclcpp::Duration(1ns), std::overflow_error("addition leads to int64_t overflow")); test_time = rclcpp::Time(INT64_MIN); RCLCPP_EXPECT_THROW_EQ( - test_time += rclcpp::Duration(-1), + test_time += rclcpp::Duration(-1ns), std::underflow_error("addition leads to int64_t underflow")); test_time = rclcpp::Time(INT64_MAX); RCLCPP_EXPECT_THROW_EQ( - test_time -= rclcpp::Duration(-1), + test_time -= rclcpp::Duration(-1ns), std::overflow_error("time subtraction leads to int64_t overflow")); test_time = rclcpp::Time(INT64_MIN); RCLCPP_EXPECT_THROW_EQ( - test_time -= rclcpp::Duration(1), + test_time -= rclcpp::Duration(1ns), std::underflow_error("time subtraction leads to int64_t underflow")); RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Duration(INT64_MAX) + rclcpp::Time(1), + test_time = rclcpp::Duration::from_nanoseconds(INT64_MAX) + rclcpp::Time(1), std::overflow_error("addition leads to int64_t overflow")); RCLCPP_EXPECT_THROW_EQ( - test_time = rclcpp::Duration(INT64_MIN) + rclcpp::Time(-1), + test_time = rclcpp::Duration::from_nanoseconds(INT64_MIN) + rclcpp::Time(-1), std::underflow_error("addition leads to int64_t underflow")); } From 31db232608f5fa8569ac36f22b5f0a73a8d2f1e2 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 30 Oct 2020 17:59:41 -0300 Subject: [PATCH 2/6] uncrustify Signed-off-by: Ivan Santiago Paunovic --- rclcpp/test/rclcpp/test_duration.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/rclcpp/test/rclcpp/test_duration.cpp b/rclcpp/test/rclcpp/test_duration.cpp index 1c608faa61..f3cbdffa3c 100644 --- a/rclcpp/test/rclcpp/test_duration.cpp +++ b/rclcpp/test/rclcpp/test_duration.cpp @@ -143,8 +143,9 @@ TEST_F(TestDuration, from_seconds) { EXPECT_EQ(rclcpp::Duration(0ns), rclcpp::Duration::from_seconds(0.0)); EXPECT_EQ(rclcpp::Duration(0ns), rclcpp::Duration::from_seconds(0)); EXPECT_EQ(rclcpp::Duration(1, HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(1.5)); - EXPECT_EQ(rclcpp::Duration::from_nanoseconds( - -ONE_AND_HALF_SEC_IN_NS), rclcpp::Duration::from_seconds(-1.5)); + EXPECT_EQ( + rclcpp::Duration::from_nanoseconds(-ONE_AND_HALF_SEC_IN_NS), + rclcpp::Duration::from_seconds(-1.5)); } TEST_F(TestDuration, std_chrono_constructors) { @@ -257,7 +258,7 @@ TEST_F(TestDuration, test_some_exceptions) { rclcpp::Duration test_duration(0ns); RCLCPP_EXPECT_THROW_EQ( test_duration = - rclcpp::Duration::from_nanoseconds(INT64_MAX) - rclcpp::Duration(-1ns), + rclcpp::Duration::from_nanoseconds(INT64_MAX) - rclcpp::Duration(-1ns), std::overflow_error("duration subtraction leads to int64_t overflow")); RCLCPP_EXPECT_THROW_EQ( test_duration = test_duration * (std::numeric_limits::infinity()), From d3bf0affee06aec075ceb587f804a67179831699 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 2 Nov 2020 10:34:02 -0300 Subject: [PATCH 3/6] More instructive deprecation message Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/duration.hpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/duration.hpp b/rclcpp/include/rclcpp/duration.hpp index b350821a10..ff44f70fb9 100644 --- a/rclcpp/include/rclcpp/duration.hpp +++ b/rclcpp/include/rclcpp/duration.hpp @@ -39,7 +39,10 @@ class RCLCPP_PUBLIC Duration Duration(int32_t seconds, uint32_t nanoseconds); /// Construct duration from the specified nanoseconds. - [[deprecated("Use Duration::from_nanoseconds instead")]] + [[deprecated( + "Use Duration::from_nanoseconds instead or std::chrono literals. For example:" + "rclcpp::Duration::from_nanoseconds(int64_variable);" + "rclcpp::Duration(0ns);")]] explicit Duration(rcl_duration_value_t nanoseconds); /// Construct duration from the specified std::chrono::nanoseconds. From fa216a67719b2c598f9a2e54a516938403bf98f4 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 2 Nov 2020 11:52:07 -0300 Subject: [PATCH 4/6] std::chrono literals -> std::chrono_literals Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/duration.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/duration.hpp b/rclcpp/include/rclcpp/duration.hpp index ff44f70fb9..e5d7c5e130 100644 --- a/rclcpp/include/rclcpp/duration.hpp +++ b/rclcpp/include/rclcpp/duration.hpp @@ -40,7 +40,7 @@ class RCLCPP_PUBLIC Duration /// Construct duration from the specified nanoseconds. [[deprecated( - "Use Duration::from_nanoseconds instead or std::chrono literals. For example:" + "Use Duration::from_nanoseconds instead or std::chrono_literals. For example:" "rclcpp::Duration::from_nanoseconds(int64_variable);" "rclcpp::Duration(0ns);")]] explicit Duration(rcl_duration_value_t nanoseconds); From 505c516ffe4c69fa69bb84d09d9f3f16a28b543d Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 2 Nov 2020 14:21:06 -0300 Subject: [PATCH 5/6] fix argument name Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/src/rclcpp/duration.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/duration.cpp b/rclcpp/src/rclcpp/duration.cpp index 383ad6499e..cd7c3e362b 100644 --- a/rclcpp/src/rclcpp/duration.cpp +++ b/rclcpp/src/rclcpp/duration.cpp @@ -255,7 +255,7 @@ Duration::from_seconds(double seconds) } Duration -Duration::from_nanoseconds(rcl_duration_value_t seconds) +Duration::from_nanoseconds(rcl_duration_value_t nanoseconds) { Duration ret; ret.rcl_duration_.nanoseconds = seconds; From 6829a4f27bd0320e89ef9778a999bbd7390fdf07 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 2 Nov 2020 14:26:52 -0300 Subject: [PATCH 6/6] fix naming Signed-off-by: Ivan Santiago Paunovic --- rclcpp/src/rclcpp/duration.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/src/rclcpp/duration.cpp b/rclcpp/src/rclcpp/duration.cpp index cd7c3e362b..40ce9daaa9 100644 --- a/rclcpp/src/rclcpp/duration.cpp +++ b/rclcpp/src/rclcpp/duration.cpp @@ -258,7 +258,7 @@ Duration Duration::from_nanoseconds(rcl_duration_value_t nanoseconds) { Duration ret; - ret.rcl_duration_.nanoseconds = seconds; + ret.rcl_duration_.nanoseconds = nanoseconds; return ret; }