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

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

Merged
merged 2 commits into from
Jan 31, 2019

Conversation

meyerj
Copy link
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
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
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
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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants