Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Add hasStarted() const to WallTimer and SteadyTimer API#1565

Merged
dirk-thomas merged 2 commits intoros:melodic-develfrom
meyerj:extend_timer_api-2
Jan 31, 2019
Merged

Add hasStarted() const to WallTimer and SteadyTimer API#1565
dirk-thomas merged 2 commits intoros:melodic-develfrom
meyerj:extend_timer_api-2

Conversation

@meyerj
Copy link
Copy Markdown
Contributor

@meyerj meyerj commented Dec 14, 2018

Completes #1464 (Add hasStarted() const to Timer API):
Also add hasStarted() method to ros::WallTimer and ros::SteadyTimer, such that the timer APIs are identical.

Whitespace changes are the result of merging steady_timer.cpp with timer.cpp and wall_timer.cpp. In a future release it would be possible to define a template class TimerBase<T,D,E> and three typedefs for Timer, WallTimer and SteadyTimer to avoid code duplication.

The patch is fully ABI-compatible because it only adds two new non-virtual member functions.

Addresses #1557 (comment):

I guess it would actually make sense to create a separate PR for the added hasStarted methods...
They have nothing to do with the rest and can then more easily be reviewed/merged separately.

@dirk-thomas
Copy link
Copy Markdown
Member

Please avoid unrelated white space changes as well as moving code unnecessarily. Both just make backporting changes across branches more difficult.

@meyerj
Copy link
Copy Markdown
Contributor Author

meyerj commented Jan 31, 2019

Please avoid unrelated white space changes as well as moving code unnecessarily. Both just make backporting changes across branches more difficult.

The goal was to unify whitespace between steady_timer.cpp with timer.cpp and wall_timer.cpp, which are almost identical and only have been copied over the years. Some patches, like #1464, have only been applied to one of them, although they actually apply to all three implementations. The unified whitespace helps to compare those three on the same branch.

I will minimize the diff as requested.

…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.
@dirk-thomas
Copy link
Copy Markdown
Member

Thank you for the patch and for iterating on it.

The CI failures are due to tests known to be flaky 😞

@dirk-thomas dirk-thomas merged commit 795adf6 into ros:melodic-devel Jan 31, 2019
tahsinkose pushed a commit to tahsinkose/ros_comm that referenced this pull request Apr 15, 2019
* 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).
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants