Add TIME_MAX and DURATION_MAX functions #538
Conversation
|
The PR currently has merge conflicts. After resolving them please also run the tests of the package and address any linter warnings. |
rclcpp/src/rclcpp/duration.cpp
Outdated
| Duration | ||
| Duration::DURATION_MAX() | ||
| { | ||
| Duration duration_max(std::numeric_limits<int32_t>::max(), 999999999); |
There was a problem hiding this comment.
@tfoote This PR and #524 say a duration is limited by the max value that can be given to the constructor accepting seconds and nanoseconds, but there is another constructor taking nanoseconds directly, and that seems to accept a number of nanoseconds as large as the 64bit rcutils type. Should this instead be std::numeric_limits<rcl_duration_value_t>::max()? Alternatively, should the nanoseconds constructor restrict the maximum size to int32_t seconds + 999999999 nanoseconds?
There was a problem hiding this comment.
I think that the nanoseconds constructor should limit it to this representation. Otherwise we'll overflow the message datastructure.
rclcpp/src/rclcpp/time.cpp
Outdated
| Time | ||
| TIME_MAX() | ||
| { | ||
| Time time_max(std::numeric_limits<int32_t>::max(), 999999999); |
There was a problem hiding this comment.
This seems like it would be easier to read as a one line function. No need to name the temporary variable, it could just be returned.
rclcpp/src/rclcpp/duration.cpp
Outdated
| Duration | ||
| Duration::DURATION_MAX() | ||
| { | ||
| Duration duration_max(std::numeric_limits<int32_t>::max(), 999999999); |
There was a problem hiding this comment.
This seems like it would be easier to read as a one line function. No need to name the temporary variable, it could just be returned.
rclcpp/include/rclcpp/duration.hpp
Outdated
| @@ -89,6 +89,10 @@ class Duration | |||
| Duration | |||
| operator-(const rclcpp::Duration & rhs) const; | |||
|
|
|||
| RCLCPP_PUBLIC | |||
| static Duration | |||
| DURATION_MAX(); | |||
There was a problem hiding this comment.
Please rename the method to use lowercase (as per code style). Also since it is defined in the class Duration I don't think it needs to be part of the name. So I would suggest just calling it max() which will be used as Duration::max().
Same for time below.
|
Thank you for the patch and for iterating on it. |
I've exposed the existing function as public API and added documentation and unit tests. Signed-off-by: Jacob Perron <jacob@openrobotics.org>
…nfo (ros2#538) Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Related to issue #524 .
-> I am not sure about the tests which I added to validate the functions.Would require some help in them.
-> I have done the linting checks locally in my workstation.