Definition of math::clock

[chapulina]

Environment

• OS Version: all
• Source or binary build? all

Description

Expected behavior

Maybe it's just me, but I was surprised when I noticed that there's an ignition::math::clock class, defined here:

gz-math/include/ignition/math/Stopwatch.hh

Line 31 in 49c3915

using clock = std::chrono::steady_clock;

and used for example here:

gz-math/src/Stopwatch_TEST.cc

Lines 45 to 46 in 49c3915

	EXPECT_EQ(ignition::math::clock::duration::zero(),
	_time.ElapsedStopTime());

I think it's dangerous to define a clock directly under the math namespace in a specific public header like this. It can collide with other clocks defined in other headers.

Actual behavior:

I'd expect that alias to be either:

• Internal to the StopWatch class and not visible through the public header
• Defined in a more global place like Helpers.hh if we want that to be the default clock type used throughout all gz-math classes.

Steps to reproduce

Example on how to cause a compilation error:

diff --git a/include/ignition/math/Helpers.hh b/include/ignition/math/Helpers.hh
index 7152b522..35d83c1f 100644
--- a/include/ignition/math/Helpers.hh
+++ b/include/ignition/math/Helpers.hh
@@ -220,6 +220,7 @@ namespace ignition
   /// \brief Math classes and function useful in robot applications.
   namespace math
   {
+    using clock = std::chrono::system_clock;
     // Inline bracket to help doxygen filtering.
     inline namespace IGNITION_MATH_VERSION_NAMESPACE {
     //
diff --git a/src/Stopwatch_TEST.cc b/src/Stopwatch_TEST.cc
index 5df56321..2db84f37 100644
--- a/src/Stopwatch_TEST.cc
+++ b/src/Stopwatch_TEST.cc
@@ -18,6 +18,7 @@
 #include <gtest/gtest.h>
 #include <thread>
 
+#include "ignition/math/Helpers.hh"
 #include "ignition/math/Stopwatch.hh"
 
 using namespace ignition;

Suggestion

At this point, removing that definition would break API. Considering that steady_clock is the only clock type used throughout the library (so far), we could consider moving that alias to Helper.hh so that it can be used by classes other than StopWatch, i.e. SpeedLimiter, DiffDriveOdometry, GaussMarkovProcess...

Alternatively, we could deprecate ignition::math::clock from version 7 and encourage downstream users to use std::chrono or define their own aliases as they see fit.

[scpeters]

it's also used in gz-sim in a couple places:

• https://github.com/gazebosim/gz-sim/blob/b753afed7b765a217260b1cdc8ee756ba94bbdc9/src/SimulationRunner.hh#L283-L287
• https://github.com/gazebosim/gz-sim/blob/ign-gazebo6/src/systems/odometry_publisher/OdometryPublisher.cc#L98

[scpeters]

it's probably better to encourage people to use std::chrono::steady_clock directly and deprecate this alias in math7

@nkoenig you have touched this code in the past; does that seem reasonable?

[scpeters]

looks like some math::clock references were removed from gz-sim in gazebosim/gz-sim#1526

[scpeters]

deprecating math::clock in #600