-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark rclcpp::Clock::now() as const #2050
Conversation
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable to me with green CI (though I think we should run tests with --packages-above rclcpp
, just to make sure no downstream packages have a problem with this).
What might happen downstream at run time that needs to be tested? |
Actually, that's a good point. I missed that the build was |
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org> Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
I think
now()
can be marked asconst
. All it does is call rcl_clock_get_now() which doesn't look like it modifies the clock to me.get_now
isrcl_get_ros_time
: https://github.com/ros2/rcl/blob/e4f7e1367dfda83d3e309397f9201642ebe38618/rcl/src/rcl/time.c#L65-L73get_now
isrcl_get_system_time
: https://github.com/ros2/rcl/blob/e4f7e1367dfda83d3e309397f9201642ebe38618/rcl/src/rcl/time.c#L44-L48get_now
isrcl_get_steady_time
: https://github.com/ros2/rcl/blob/e4f7e1367dfda83d3e309397f9201642ebe38618/rcl/src/rcl/time.c#L36-L40