Check for negative time in rclcpp::Time(int64_t nanoseconds, ...) constructor

sharminramli commented 2 months ago

rclcpp::Time currently allows a negative time when constructing with int64_t nanoseconds (but not with int32_t seconds even though both can possibly hold a negative value). For a more uniform interface and based off of https://github.com/ros2/rclcpp/issues/525 where time cannot be negative:

Added the negative time check and throw from the rclcpp::Time(int32_t seconds, uint32_t nanoseconds, ...) constructor to the rclcpp::Time(int64_t nanoseconds, ...) constructor
Added a test to check that it throws with negative time
Removed the tests which allowed for negative time (the cases should already be covered by the other tests?)

Closes https://github.com/ros2/rclcpp/issues/2507.

ahcorde commented 2 months ago

Linux
Linux-aarch64
Windows

ahcorde commented 2 months ago

@clalancette can we backport this to jazzy ? or we should wait the frozen period?

clalancette commented 2 months ago

@clalancette can we backport this to jazzy ? or we should wait the frozen period?

We are open for backports now. My only question is whether we should actually backport it to Jazzy or not. We are not allowing new features into Jazzy, but we are allowing bugfixes. The other thing we want to avoid at this point is too much breakage to downstream packages.

What do you think @ahcorde ? Do you consider this a bugfix or a feature, and what do you think the downstream impact is going to be?

fujitatomoya commented 2 months ago

@clalancette @ahcorde IMO, this could possibly break the user application. so i would not backport this to released distros...

ros2 / rclcpp

Check for negative time in rclcpp::Time(int64_t nanoseconds, ...) constructor #2510