From d3421601df949e029cb8d35047761c3f9af2391f Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Thu, 16 Sep 2021 14:46:35 +0800 Subject: [PATCH 1/7] Add new interface for time conversion Signed-off-by: Barry Xu --- CMakeLists.txt | 3 ++ include/rcpputils/time.hpp | 62 ++++++++++++++++++++++++++++++++++++++ test/test_time.cpp | 28 +++++++++++++++++ 3 files changed, 93 insertions(+) create mode 100644 include/rcpputils/time.hpp create mode 100644 test/test_time.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 68e8e27..78155d9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -69,6 +69,9 @@ if(BUILD_TESTING) ament_add_gtest(test_join test/test_join.cpp) + ament_add_gtest(test_time test/test_time.cpp) + ament_target_dependencies(test_time rcutils) + ament_add_gtest(test_env test/test_env.cpp ENV EMPTY_TEST= diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp new file mode 100644 index 0000000..bae36ff --- /dev/null +++ b/include/rcpputils/time.hpp @@ -0,0 +1,62 @@ +// Copyright 2021 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. + +#ifndef RCPPUTILS__TIME_HPP_ +#define RCPPUTILS__TIME_HPP_ + +#include + +#include "rcutils/time.h" + +#include "rcpputils/visibility_control.hpp" + +namespace rcpputils +{ + +/// Convert to rcutils duration value. +/* + * \param[in] time The time to be converted to rcutils duration. + * \return rcutils duration value + * \throws std::invalid_argument if time is bigger than std::chrono::nanoseconds::max(). + */ +template +RCPPUTILS_PUBLIC +rcutils_duration_value_t convert_to_rcutils_duration( + const std::chrono::duration & time) +{ + // Casting to a double representation might lose precision and allow the check below to succeed + // but the actual cast to nanoseconds fail. Using 1 DurationT worth of nanoseconds less than max + constexpr auto maximum_safe_cast_ns = + std::chrono::nanoseconds::max() - std::chrono::duration(1); + // If period is greater than nanoseconds::max(), the duration_cast to nanoseconds will overflow + // a signed integer, which is undefined behavior. Checking whether any std::chrono::duration is + // greater than nanoseconds::max() is a difficult general problem. This is a more conservative + // version of Howard Hinnant's (the guy>) response here: + // https://stackoverflow.com/a/44637334/2089061 + // However, this doesn't solve the issue for all possible duration types of period. + // Follow-up issue: https://github.com/ros2/rclcpp/issues/1177 + constexpr auto ns_max_as_double = + std::chrono::duration_cast>( + maximum_safe_cast_ns); + if (time > ns_max_as_double) { + throw std::invalid_argument{ + "time must be less than std::chrono::nanoseconds::max()"}; + } + + return (std::chrono::duration_cast(time)).count(); +} + +} // namespace rcpputils + +#endif // RCPPUTILS__TIME_HPP_ diff --git a/test/test_time.cpp b/test/test_time.cpp new file mode 100644 index 0000000..c76f1d9 --- /dev/null +++ b/test/test_time.cpp @@ -0,0 +1,28 @@ +// Copyright 2021 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 + +#include + +TEST(test_time, test_convert_to_rcutils_duration) { + rcutils_duration_value_t expect_value = RCUTILS_S_TO_NS(5 * 60); // 5 minutes + rcutils_duration_value_t cast_val; + EXPECT_NO_THROW(cast_val = rcpputils::convert_to_rcutils_duration(std::chrono::minutes(5))); + EXPECT_EQ(cast_val, expect_value); + + EXPECT_THROW( + rcpputils::convert_to_rcutils_duration(std::chrono::hours(10000000)), + std::invalid_argument); +} From fbd058132fe21d84117d4fc46877a2331818256c Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Fri, 17 Sep 2021 13:26:57 +0800 Subject: [PATCH 2/7] Change return type and function name Signed-off-by: Barry Xu --- include/rcpputils/time.hpp | 15 +++++++++------ test/test_time.cpp | 6 +++--- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp index bae36ff..db2e1ed 100644 --- a/include/rcpputils/time.hpp +++ b/include/rcpputils/time.hpp @@ -24,15 +24,18 @@ namespace rcpputils { -/// Convert to rcutils duration value. -/* - * \param[in] time The time to be converted to rcutils duration. - * \return rcutils duration value +/// Convert to std::chrono::nanoseconds. +/** + * This function help to convert from std::chrono::duration to std::chrono::nanoseconds and throw + * exception if overflow occurs while coverting. + * + * \param[in] time The time to be converted to std::chrono::nanoseconds. + * \return std::chrono::nanoseconds. * \throws std::invalid_argument if time is bigger than std::chrono::nanoseconds::max(). */ template RCPPUTILS_PUBLIC -rcutils_duration_value_t convert_to_rcutils_duration( +std::chrono::nanoseconds convert_to_nanoseconds( const std::chrono::duration & time) { // Casting to a double representation might lose precision and allow the check below to succeed @@ -54,7 +57,7 @@ rcutils_duration_value_t convert_to_rcutils_duration( "time must be less than std::chrono::nanoseconds::max()"}; } - return (std::chrono::duration_cast(time)).count(); + return std::chrono::duration_cast(time); } } // namespace rcpputils diff --git a/test/test_time.cpp b/test/test_time.cpp index c76f1d9..27a39f6 100644 --- a/test/test_time.cpp +++ b/test/test_time.cpp @@ -16,13 +16,13 @@ #include -TEST(test_time, test_convert_to_rcutils_duration) { +TEST(test_time, test_convert_to_nanoseconds) { rcutils_duration_value_t expect_value = RCUTILS_S_TO_NS(5 * 60); // 5 minutes rcutils_duration_value_t cast_val; - EXPECT_NO_THROW(cast_val = rcpputils::convert_to_rcutils_duration(std::chrono::minutes(5))); + EXPECT_NO_THROW(cast_val = rcpputils::convert_to_nanoseconds(std::chrono::minutes(5)).count()); EXPECT_EQ(cast_val, expect_value); EXPECT_THROW( - rcpputils::convert_to_rcutils_duration(std::chrono::hours(10000000)), + rcpputils::convert_to_nanoseconds(std::chrono::hours(10000000)), std::invalid_argument); } From 6bf8e51c667e99943bc622774997340f51360703 Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Wed, 22 Sep 2021 21:13:06 +0800 Subject: [PATCH 3/7] Fix build error on Windows and MacOS Signed-off-by: Barry Xu --- include/rcpputils/time.hpp | 3 --- test/test_time.cpp | 2 +- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp index db2e1ed..f017b41 100644 --- a/include/rcpputils/time.hpp +++ b/include/rcpputils/time.hpp @@ -19,8 +19,6 @@ #include "rcutils/time.h" -#include "rcpputils/visibility_control.hpp" - namespace rcpputils { @@ -34,7 +32,6 @@ namespace rcpputils * \throws std::invalid_argument if time is bigger than std::chrono::nanoseconds::max(). */ template -RCPPUTILS_PUBLIC std::chrono::nanoseconds convert_to_nanoseconds( const std::chrono::duration & time) { diff --git a/test/test_time.cpp b/test/test_time.cpp index 27a39f6..fc2cbed 100644 --- a/test/test_time.cpp +++ b/test/test_time.cpp @@ -18,7 +18,7 @@ TEST(test_time, test_convert_to_nanoseconds) { rcutils_duration_value_t expect_value = RCUTILS_S_TO_NS(5 * 60); // 5 minutes - rcutils_duration_value_t cast_val; + rcutils_duration_value_t cast_val = 0; EXPECT_NO_THROW(cast_val = rcpputils::convert_to_nanoseconds(std::chrono::minutes(5)).count()); EXPECT_EQ(cast_val, expect_value); From 26f299bee60a062ea78adcd93114ce6269d3107e Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Sat, 9 Oct 2021 13:33:00 +0800 Subject: [PATCH 4/7] Add check for the minimum Signed-off-by: Barry Xu --- include/rcpputils/time.hpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp index f017b41..e2e1fd5 100644 --- a/include/rcpputils/time.hpp +++ b/include/rcpputils/time.hpp @@ -29,7 +29,8 @@ namespace rcpputils * * \param[in] time The time to be converted to std::chrono::nanoseconds. * \return std::chrono::nanoseconds. - * \throws std::invalid_argument if time is bigger than std::chrono::nanoseconds::max(). + * \throws std::invalid_argument if time is bigger than std::chrono::nanoseconds::max() or less than + * std::chrono::nanoseconds::min(). */ template std::chrono::nanoseconds convert_to_nanoseconds( @@ -54,6 +55,14 @@ std::chrono::nanoseconds convert_to_nanoseconds( "time must be less than std::chrono::nanoseconds::max()"}; } + constexpr auto ns_min_as_double = + std::chrono::duration_cast>( + std::chrono::nanoseconds::min()); + if (time < ns_min_as_double) { + throw std::invalid_argument{ + "time must be bigger than std::chrono::nanoseconds::min()"}; + } + return std::chrono::duration_cast(time); } From f45f6d4c0478e750f1edd2edef00c0d5bd66947a Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Tue, 26 Oct 2021 10:25:39 +0800 Subject: [PATCH 5/7] Add example in comments and update test code Signed-off-by: Barry Xu --- include/rcpputils/time.hpp | 1 + test/test_time.cpp | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp index e2e1fd5..9895711 100644 --- a/include/rcpputils/time.hpp +++ b/include/rcpputils/time.hpp @@ -46,6 +46,7 @@ std::chrono::nanoseconds convert_to_nanoseconds( // version of Howard Hinnant's (the guy>) response here: // https://stackoverflow.com/a/44637334/2089061 // However, this doesn't solve the issue for all possible duration types of period. + // e.g std::chrono::hours(10000000) cannot be converted to nanoseconds. // Follow-up issue: https://github.com/ros2/rclcpp/issues/1177 constexpr auto ns_max_as_double = std::chrono::duration_cast>( diff --git a/test/test_time.cpp b/test/test_time.cpp index fc2cbed..7f30ef3 100644 --- a/test/test_time.cpp +++ b/test/test_time.cpp @@ -19,7 +19,8 @@ TEST(test_time, test_convert_to_nanoseconds) { rcutils_duration_value_t expect_value = RCUTILS_S_TO_NS(5 * 60); // 5 minutes rcutils_duration_value_t cast_val = 0; - EXPECT_NO_THROW(cast_val = rcpputils::convert_to_nanoseconds(std::chrono::minutes(5)).count()); + EXPECT_NO_THROW( + cast_val = rcpputils::convert_to_nanoseconds(std::chrono::duration(5 * 60)).count()); EXPECT_EQ(cast_val, expect_value); EXPECT_THROW( From d810a35d45b7fcc4457c153b48f503954fae57e7 Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Mon, 15 Nov 2021 15:47:13 +0800 Subject: [PATCH 6/7] The value of maximum_safe_cast_ns doesn't depend on user input. Signed-off-by: Barry Xu --- include/rcpputils/time.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp index 9895711..3a8d164 100644 --- a/include/rcpputils/time.hpp +++ b/include/rcpputils/time.hpp @@ -37,9 +37,9 @@ std::chrono::nanoseconds convert_to_nanoseconds( const std::chrono::duration & time) { // Casting to a double representation might lose precision and allow the check below to succeed - // but the actual cast to nanoseconds fail. Using 1 DurationT worth of nanoseconds less than max + // but the actual cast to nanoseconds fail. Using 1 worth of nanoseconds less than max constexpr auto maximum_safe_cast_ns = - std::chrono::nanoseconds::max() - std::chrono::duration(1); + std::chrono::nanoseconds::max() - std::chrono::nanoseconds(1); // If period is greater than nanoseconds::max(), the duration_cast to nanoseconds will overflow // a signed integer, which is undefined behavior. Checking whether any std::chrono::duration is // greater than nanoseconds::max() is a difficult general problem. This is a more conservative From e9f73af8c0679d2f2f45ba4703d2b69e8d87be5c Mon Sep 17 00:00:00 2001 From: Barry Xu Date: Wed, 17 Nov 2021 18:01:10 +0800 Subject: [PATCH 7/7] Address comments Signed-off-by: Barry Xu --- include/rcpputils/time.hpp | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/include/rcpputils/time.hpp b/include/rcpputils/time.hpp index 3a8d164..073f72f 100644 --- a/include/rcpputils/time.hpp +++ b/include/rcpputils/time.hpp @@ -32,25 +32,13 @@ namespace rcpputils * \throws std::invalid_argument if time is bigger than std::chrono::nanoseconds::max() or less than * std::chrono::nanoseconds::min(). */ -template +template std::chrono::nanoseconds convert_to_nanoseconds( const std::chrono::duration & time) { - // Casting to a double representation might lose precision and allow the check below to succeed - // but the actual cast to nanoseconds fail. Using 1 worth of nanoseconds less than max - constexpr auto maximum_safe_cast_ns = - std::chrono::nanoseconds::max() - std::chrono::nanoseconds(1); - // If period is greater than nanoseconds::max(), the duration_cast to nanoseconds will overflow - // a signed integer, which is undefined behavior. Checking whether any std::chrono::duration is - // greater than nanoseconds::max() is a difficult general problem. This is a more conservative - // version of Howard Hinnant's (the guy>) response here: - // https://stackoverflow.com/a/44637334/2089061 - // However, this doesn't solve the issue for all possible duration types of period. - // e.g std::chrono::hours(10000000) cannot be converted to nanoseconds. - // Follow-up issue: https://github.com/ros2/rclcpp/issues/1177 constexpr auto ns_max_as_double = std::chrono::duration_cast>( - maximum_safe_cast_ns); + std::chrono::nanoseconds::max()); if (time > ns_max_as_double) { throw std::invalid_argument{ "time must be less than std::chrono::nanoseconds::max()"};