Explicit equality operator between rclcpp::Time and msg::Time

[AiVerisimilitude]

Post C++20 will see automatic comparison operator creation, which leads to ambiguity errors on comparisons between rclcpp::Time and builtin_interfaces::msg::Time.

A simple fix would be to cast the message type to rclcpp::Time. However, this equality comparison can be numerous within a codebase so this PR takes a different approach by instead providing an explicit equality operator between the two types thus fixing this issue codebase wide. Let me know if this is a desirable approach, or if we should instead require the explicit casts on these operations after migrating to C++20.

One liberty I took was to make this comparison independent of the clock type. My reasoning is that msg::Time does not have a clock type and rclcpp::Time constructor from msg::Time does not assume a fixed clock type (can be set).

[clalancette]

Let me know if this is a desirable approach, or if we should instead require the explicit casts on these operations after migrating to C++20.

We discussed this a bit, and we think that to be clear users should have to cast it themselves. That would make it clear which time source they are comparing against. That is, users would have to explicitly do:

rclcpp_time == Time(my_msg_time, get_clock_type())

Looking at this code, one other thing we are noticing is that there is an rclcpp::Time constructor that takes a builtin_interfaces::msg::Time. That should probably be marked explicit, though that might cause some downstream breakage. Maybe we should instead delete the == comparison operator between rclcpp::Time and builtin_msgs::msg::Time? Thoughts?

[AiVerisimilitude]

Marking the constructor of rclcpp::Time that takes a builtin_interfaces::msg::Time would also fix it, since the issue stems from the different conversion options given by rclcpp::Time. In C++20 the ambiguity comes from being able to use either the implicit constructor or the conversion operator.

So for this comparison:

rclcpp::Time time{};
builtin_interfaces::msg::Time msg{};

bool res = time == msg

In C++17 the behavior will be to convert msg into rclcpp::Time using the implicit constructor and then use the comparison operator from rclcpp::Time while in C++20 a new option appears where he can instead use the conversion operator of time and then use the comparison operator of builtin_interfaces::msg::Time.

Regardless the outcome for this particular issue, I do think marking the constructor explicit is the way to go, but that raises issues with initializing assignments of the form rclcpp::Time A = builtin_interfaces::msg::Time{}; since it does not use the assignment operator.

Coming back to this issue, I can indeed create the comparison operator between rclcpp::Time and builtin_interfaces::msg::Time as deleted, though this will cause some issues downstream as well. I will go around and fix them for ROS at least. Or do you think marking it as deprecated is enough until full removal on a LTS release? Would need to keep track of it, which could be error-prone.

[fujitatomoya]

@AiVerisimilitude thank you for sharing good information.

Regardless the outcome for this particular issue, I do think marking the constructor explicit is the way to go, but that raises issues with initializing assignments of the form rclcpp::Time A = builtin_interfaces::msg::Time{};

correct, this breaks the user application. IMO, this is the way to notify the user application that explicitly specify the clock source when they are creating rclcpp::Time object from builtin_interfaces::msg::Time.

In C++17 the behavior will be to convert msg into rclcpp::Time using the implicit constructor

so that this can be avoided.

while in C++20 a new option appears where he can instead use the conversion operator of time and then use the comparison operator of builtin_interfaces::msg::Time.

i think this process itself is okay, since it uses conversion operator 1st and user specifies so? i really do not have good practice for this option, could you share information about this option?

[AiVerisimilitude]

correct, this breaks the user application. IMO, this is the way to notify the user application that explicitly specify the clock source when they are creating rclcpp::Time object from builtin_interfaces::msg::Time

Just to be clear, the issue with this code: rclcpp::Time A = builtin_interfaces::msg::Time{}; which is in the context of construction is different from

rclcpp::Time A;
A = builtin_interfaces::msg::Time{};

which uses the assignment operator and does not allow the user to specify the clock type, since it calls the constructor with the default clock:

Time &
Time::operator=(const builtin_interfaces::msg::Time & time_msg)
{
  *this = Time(time_msg);
  return *this;
}

I think that with the direction that we are going with explicit clocks that I would need to remove/delete anyway, so the user has to specify the clock type. Otherwise, the clock type when using this assignment operator will always be set to the default one.

i think this process itself is okay, since it uses conversion operator 1st and user specifies so?

Since the conversion is also implicit, you don't need to specify so. However, the conversion is to builtin_interfaces::msg::Time which does not have a clock type, so I guess it's not as bad?

For clarity, in C++20 with having the constructor of rclcpp::Time that takes a builtin_interfaces::msg::Time marked as explicit this comparison:

rclcpp::Time time{};
builtin_interfaces::msg::Time msg{};

bool res = time == msg

Uses rclcpp::Time implicit conversion operator to builtin_interfaces::msg::Time to convert time to a msg and then make the comparison there. To avoid that, we would need to also mark the conversion operator explicit.

Another option would be to delete the comparison operator of builtin_interfaces::msg::Time == builtin_interfaces::msg::Time since that fixes the ambiguity and makes it so that all comparisons have to be of rclcpp::Time (we would still need to make the other changes), but I do not know how feasible that is (have not yet looked at that part of the codebase).

I will make a couple of changes and see how it looks :)

Thanks for the discourse/engagement in this topic ^_^

[fujitatomoya]

Since the conversion is also implicit, you don't need to specify so. However, the conversion is to builtin_interfaces::msg::Time which does not have a clock type, so I guess it's not as bad?

that was what i meant here. conversion from rclcpp::Time to builtin_interfaces::msg::Time, because user knows that there is no clock type.

and with

while in C++20 a new option appears where he can instead use the conversion operator of time and then use the comparison operator of builtin_interfaces::msg::Time.

i was thinking this behavior is option that user can enable during compile time? i guess that was wrong assumption.

To avoid that, we would need to also mark the conversion operator explicit.

IMO this is okay because user knows that they are comparing builtin_interfaces::msg::Time? but this still may be ambiguity.

Another option would be to delete the comparison operator of builtin_interfaces::msg::Time == builtin_interfaces::msg::Time since that fixes the ambiguity and makes it so that all comparisons have to be of rclcpp::Time

i am inclined to take this path, so that there is no ambiguity for user application.

@clalancette @alsora @wjwwood @ros2/team any thoughts?

[AiVerisimilitude]

This would be my new proposal given the discussion so far.

1. I've made the constructor of rclcpp::Time from builtin_interfaces::msg::Time explicit. This means there is no longer any implicit conversion support and assignments while constructing will also need to be changed. For now, I kept the behavior where there is a default clock. Let me know if this should also be made explicit.

This change means that this

builtin_interfaces::msg::Time msg;

rclcpp::Time time = msg;

will no longer be supported and can instead be:

rclcpp::Time time(msg);
//or
rclcpp::Time time(...);
time = msg;

since I did not make the assignment operator explicit. If we want to make the assignment explicit, I would instead suggest not providing one and require the construction of a rclcpp::Time first and then assign it:

rclcpp::Time time(..);

time = rclcpp::Time(msg);

2. The assignment operator from builtin_interfaces::msg::Time no longer overrides the current clock type. I do not know why this is the default behavior, but let me know if I should revert it. I made this change in order to support a cached time object whose clock type is not overridden by assignment from a builtin_interfaces::msg::Time.

With this change this comparison is now true:

builtin_interfaces::msg::Time msg();

rclcpp::Time a(0, RCL_SYSTEM_TIME);

a = msg;

a.get_clock_type() == RCL_SYSTEM_TIME // is TRUE since clock type in not updated by assignment from message

However, if we decide to remove the assignment operator from builtin_interfaces::msg::Time none of this would be applicable/ a problem as the Time object needs to be constructed first.

3. deleted comparison operators between rclcpp::Time and builtin_interfaces::msg::Time. For compatibility with C++17 I created the 2 operators as non-member functions instead of 1 member one (which in C++20 would be automatically reversed).

The reasoning is that I don't want automatic decay of a rclcpp::Time to builtin_interfaces::msg::Time since we have a conversion operator. With the 2 comparison operators deleted the compiler will find them to be the best fit and correctly issue a compiler error (use of deleted function). The user can however make an explicit conversion and use the equality comparison between equal types:

rclcpp::Time time(..);
builtin_interfaces::msg::Time msg(..);

time == msg // compiler error: use of deleted function
msg == time // compiler error: use of deleted function

time == rclcpp::Time(msg) // OK if clock_type of object time is the default and time in message is not negative
msg == static_cast<builtin_interfaces::msg::Time>(time) // OK comparison uses conversion operator and is clock_type agnostic

[fujitatomoya]

If we want to make the assignment explicit, I would instead suggest not providing one and require the construction of a rclcpp::Time first and then assign it

makes sense to me.

deleted comparison operators between rclcpp::Time and builtin_interfaces::msg::Time

i think this is good.

yes, this requires application code change, and assignment operator from builtin_interfaces::msg::Time behavior changes.

[AiVerisimilitude]

There has been a new version of this PR pushed for a while. I've re-requested a review, but not sure if I did it correctly since it's been a while without any feedback. Can anyone (@wjwwood @hidmic @ivanpauno @clalancette @fujitatomoya) just give me a quick confirmation that this on the radar?

On the side I've been updating other packages to be compatible with these changes and will be submitting some PR's to those repos and linking this issue to them for traceability.

[fujitatomoya]

@AiVerisimilitude this PR has been on my list for a long time, i will check this in this week.

[wjwwood]

Sorry for joining this conversation late, but I wanted to clear up a something that was brought up several times, which is the idea that builtin_interfaces::msg::Time has no clock type, but it does at least have an implied epoch, which is the same as system time or ros time. So a few things should be enforced (imo):

• when assigning a "msg::Time" into a rclcpp::Time, we should either ensure the clock type is one of system/ros, or explicitly assign it to be ros or system (I'm not sure which would be most correct)
• similarly conversion constructors and conversion operators need to make sure that we don't end up with a steady clock based rclcpp::Time from a msg::Time
• converting to a msg::Time from rclcpp::Time, for any reason including comparison, needs to make sure the source rclcpp::Time is not steady time
• static casting a rclcpp::Time to a msg::Time needs to only be allowed when the clock type is not steady time (ensured by the conversion constructor/operator characteristics mentioned above)
• if we have an explicit comparison between rclcpp::Time and msg::Time it need to ensure the rclcpp::Time is not steady clock based

With those in mind, I'll do a review of this pr soon, but in the meantime I just wanted to make that clearer.

[AiVerisimilitude]

@wjwwood friendly poke for review feedback

[AiVerisimilitude]

What is the status for review here? If there are any issues, just let me know what these are or if a larger discussion is needed, where to have it. Mostly want to know because if this will take a longer time I would like to create a separate PR that addresses only the C++20 compatibility (it would still be a bit of a workaround) so I can still dream a bit that the next release will be C++20 compatible

[fujitatomoya]

@AiVerisimilitude this still waiting for the another review. CC: @wjwwood