ros2 / rclcpp

rclcpp (ROS Client Library for C++)
Apache License 2.0
536 stars 417 forks source link

rclcpp::Time(int64_t nanoseconds, ...) should check for negative time #2507

Closed sharminramli closed 5 months ago

sharminramli commented 5 months ago

Bug report

Required Info:

Steps to reproduce issue

Create a dummy node with the following:

    RCLCPP_INFO_STREAM(rclcpp::get_logger(""), "Negative rclcpp::Time with int64_t ns");
    const auto negative_ns = rclcpp::Time(-1);
    RCLCPP_INFO_STREAM(rclcpp::get_logger(""), "Negative rclcpp::Time with int32_t s, uint32_t ns");
    const auto negative_s_ns = rclcpp::Time(-1, 0);

Expected behavior

Both negative_ns and negative_s_ns should throw.

Actual behavior

negative_ns does not throw, only negative_s_ns does:

[INFO] [1713275443.003549819] []: Negative rclcpp::Time with int64_t ns
[INFO] [1713275443.003577744] []: Negative rclcpp::Time with int32_t s, uint32_t ns
terminate called after throwing an instance of 'std::runtime_error'
  what():  cannot store a negative time point in rclcpp::Time
[ros2run]: Aborted

Additional information


Feature request

Feature description

Adding the <0 check https://github.com/ros2/rclcpp/blob/535d56f690164dcfcca145d2bc3c5fca18234631/rclcpp/src/rclcpp/time.cpp#L52 to https://github.com/ros2/rclcpp/blob/535d56f690164dcfcca145d2bc3c5fca18234631/rclcpp/src/rclcpp/time.cpp#L60 I would be happy to implement this change if this is indeed a missing feature.

Implementation considerations