Check for negative time in rclcpp::Time(int64_t nanoseconds, ...) constructor#2510
Conversation
Signed-off-by: Nursharmin Ramli <nursharminramli@gmail.com>
Signed-off-by: Nursharmin Ramli <nursharminramli@gmail.com>
fujitatomoya
left a comment
There was a problem hiding this comment.
i think this is right thing to do.
@clalancette @wjwwood what do you think?
Signed-off-by: Nursharmin Ramli <nursharminramli@gmail.com>
Signed-off-by: Nursharmin Ramli <nursharminramli@gmail.com>
Signed-off-by: Nursharmin Ramli <nursharminramli@gmail.com>
clalancette
left a comment
There was a problem hiding this comment.
This looks reasonable to me with green CI.
Let's put this on the queue to merge once Rolling is unfrozen.
|
|
@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? |
|
@clalancette @ahcorde IMO, this could possibly break the user application. so i would not backport this to released distros... |
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 #525 where time cannot be negative:
rclcpp::Time(int32_t seconds, uint32_t nanoseconds, ...)constructor to therclcpp::Time(int64_t nanoseconds, ...)constructorCloses #2507.