Skip to content
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

Allow != and == of Time objects with different sources #2087

Closed
wants to merge 1 commit into from

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 20, 2023

rclcpp::Time forbids comparisons between objects with different clock types. It raises an exception when that happens. This makes sense for <, <=, etc, but I think == and != could be defined by saying Time objects with different clock types are never equal to each other.

I think this might simplify some of the logic for handling Time with different time sources here: locusrobotics/fuse#307

@ros2/team thoughts?

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz sloretz self-assigned this Jan 20, 2023
@alsora
Copy link
Collaborator

alsora commented Jan 20, 2023

I think many users who compare times with different clocks are doing that by mistake, so having an exception in all operators make sense.

For the few use-cases where you really need to handle different clocks, it should be sufficient to use a custom comparison function that checks the clock type first, rather than modifying the core library.

@wjwwood
Copy link
Member

wjwwood commented Jan 20, 2023

I agree with @alsora. However, I would be ok with a different comparison function (like a named one, e.g. .clock_and_time_equal() or something) being added as a method on Time to reduce duplication across code bases.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am with @alsora , probably throwing exception is better for user application.

@sloretz
Copy link
Contributor Author

sloretz commented Jan 24, 2023

All feedback so far has pointed towards not allowing any kind of comparison between Time points with different clock types. Internally I received feedback that's pointed towards Time points for different clock types should have different types so that enforcement can happen at compile time. Given that direction, I'll close this PR.

@sloretz sloretz closed this Jan 24, 2023
@sloretz sloretz deleted the sloretz__inequality_accross_time_sources branch January 24, 2023 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants