Skip to content

Mark rclcpp::Clock::now() as const#2050

Merged
sloretz merged 1 commit intorollingfrom
sloretz__rclcpp__clock_now_const
Nov 23, 2022
Merged

Mark rclcpp::Clock::now() as const#2050
sloretz merged 1 commit intorollingfrom
sloretz__rclcpp__clock_now_const

Conversation

@sloretz
Copy link
Copy Markdown
Contributor

@sloretz sloretz commented Nov 22, 2022

@sloretz
Mark rclcpp::Clock::now() as const
045e634 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz requested a review from methylDragon November 22, 2022 01:52
@sloretz sloretz self-assigned this Nov 22, 2022
@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Nov 22, 2022 •
edited
Loading

CI (build: --packages-above-and-dependencies rclcpp test: --packages-select rclcpp)

  • Linux Build Status edit: infra fail - auto reran as Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status edit: infra fail - auto reran a few times, failing for more infra issues, manual re-run: Build Status

@sloretz sloretz mentioned this pull request Nov 22, 2022
Merged
clalancette
clalancette approved these changes Nov 22, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@clalancette clalancette left a 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).

@sloretz
Copy link
Copy Markdown
Contributor Author

sloretz commented Nov 22, 2022

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?

@clalancette
Copy link
Copy Markdown
Contributor

clalancette commented Nov 23, 2022

What might happen downstream at run time that needs to be tested?

Actually, that's a good point. I missed that the build was --packages-above-and-dependencies, so that's good for me. Sorry for the noise.

@sloretz sloretz merged commit e5d2047 into rolling Nov 23, 2022
@delete-merged-branch delete-merged-branch bot deleted the sloretz__rclcpp__clock_now_const branch November 23, 2022 16:44
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@sloretz
Mark rclcpp::Clock::now() as const (ros2#2050)
fb9a6a3 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request Apr 29, 2023
@sloretz
Mark rclcpp::Clock::now() as const (ros2#2050)
a9a89c5 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request May 3, 2023
@sloretz
Mark rclcpp::Clock::now() as const (ros2#2050)
9f12778 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
alsora pushed a commit to irobot-ros/rclcpp that referenced this pull request May 3, 2023
@sloretz
Mark rclcpp::Clock::now() as const (ros2#2050)
39f4468 
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@Timple
Copy link
Copy Markdown
Contributor

Timple commented Oct 12, 2025 •
edited
Loading

Can this be backported to humble?

Would save me a workaround: https://github.com/nobleo/ros2_fmt_logger/blob/14e0cb5e0407e33aaecf88787296208b896a7f17/include/ros2_fmt_logger/ros2_fmt_logger.hpp#L572..L574

@fujitatomoya
Copy link
Copy Markdown
Collaborator

fujitatomoya commented Oct 12, 2025

@Timple

this is ABI breaking change. application compiled against the old version would expect the non-const signature, so i think this would break the user space. i do not think we can backport this?

@Timple
Copy link
Copy Markdown
Contributor

Timple commented Oct 12, 2025

Ah, then nevermind. I'll just wait for the EOL 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@clalancette clalancette clalancette approved these changes

@alsora alsora alsora approved these changes

@ivanpauno ivanpauno ivanpauno approved these changes

@methylDragon methylDragon Awaiting requested review from methylDragon

@hidmic hidmic Awaiting requested review from hidmic

@wjwwood wjwwood Awaiting requested review from wjwwood

Assignees

@sloretz sloretz

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sloretz @clalancette @Timple @fujitatomoya @alsora @ivanpauno