Add hasStarted() const to Timer API #1464
Conversation
|
LGTM— have wanted this in the past. Slight preference for Either way, 👍 |
|
I am open to changing that. I was a little reluctant to add the const declaration to the function. Although that feels like the correct thing to do, I saw that the current API doesn't have any. Changing it would be breaking API (in the strict sense) so I didn't want to touch anything else. |
|
@clalancette could we get a rebuild//rerun of tests on Debian Stretch? |
|
@ros-pull-request-builder retest this please |
|
Adding const should always be safe, shouldn't it? REP9 does list "changing the const/volatile qualifiers of a member function" as a thing that causes ABI breakage, but that is surprising to me— my impression is that constness is enforced at compilation time, and is basically invisible to the linker. Apparently not. |
|
The |
|
It might be an ABI break, but it's definitely not an API break— it's making the method more permissive than previously, since it was previously only callable on non-const instances of the class, but is now callable on const instances as well. So there's no existing usage which will break at compile time by making a method const. |
|
I based my guess on how gdb often appends const to the end of function
names in the ASM but you are absolutely right in your reasoning.
|
|
@clalancette could you please point us to someone who could review/merge this? |
|
Thank you for the improvement. |
|
@tfoote can we cherry-pick this to Kinetic too? |
|
The new API is currently not selected for backporting into Lunar (see #1477). As such it wouldn't be backported to Kinetic (since that happens from the Lunar branch). Commonly we don't backport new features. If you think this would be very important (and can't be worked around in user code) please comment on the referenced ticket to request it being backported to Lunar. |
…lTimer and ros::SteadyTimer ros::Timer::hasStarted() has been added in ros#1464. The same member function should exist in the other two timer implementations, too, for completeness.
…lTimer and ros::SteadyTimer ros::Timer::hasStarted() has been added in ros#1464. The same member function should exist in the other two timer implementations, too, for completeness.
|
FYI #1541. |
…lTimer and ros::SteadyTimer ros::Timer::hasStarted() has been added in ros#1464. The same member function should exist in the other two timer implementations, too, for completeness.
* roscpp: copy hasStarted() member function from ros::Timer to ros::WallTimer and ros::SteadyTimer ros::Timer::hasStarted() has been added in #1464. The same member function should exist in the other two timer implementations, too, for completeness. * Check for nullptr in WallTimer::hasStarted() and SteadyTimer::hasStarted() Analogous to fe9479c (#1541).
* roscpp: copy hasStarted() member function from ros::Timer to ros::WallTimer and ros::SteadyTimer ros::Timer::hasStarted() has been added in ros#1464. The same member function should exist in the other two timer implementations, too, for completeness. * Check for nullptr in WallTimer::hasStarted() and SteadyTimer::hasStarted() Analogous to fe9479c (ros#1541).
To assert if a timer is running at the moment, one can hack their way around by using
isValidandsetDurationto invalid ones.I'm proposing the addition of a simple const accessor to the internal
started_field.Should I add a simple test file as well?
Can this be cherry-picked to kinetic-devel too?