[Base] some cleaning in timers#1811
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
There was a problem hiding this comment.
Code Review
This pull request refactors the timing utilities by extracting the Timer class and the nanosec_to_time_str function into dedicated header files, Timer.hpp and format_time.hpp, and introduces a new suite of unit tests. The code review identifies opportunities to improve encapsulation by making internal state members private, ensuring the std::chrono backend is properly initialized in the constructor to avoid incorrect duration calculations, and simplifying string formatting in the time conversion utility.
| public: | ||
| #if defined(USE_PLF_TIMER) | ||
| plf::nanotimer timer; ///< Internal timer | ||
| #else | ||
| std::chrono::steady_clock::time_point t_start; ///< Internal timer | ||
| #endif | ||
| f64 nanosec; ///< Time in nanoseconds |
There was a problem hiding this comment.
The internal state of the Timer class (the backend timer/time_point and the nanosec accumulator) is currently exposed as public members. To improve encapsulation and prevent accidental modification of the timer's state from outside the class, these should be made private.
| public: | |
| #if defined(USE_PLF_TIMER) | |
| plf::nanotimer timer; ///< Internal timer | |
| #else | |
| std::chrono::steady_clock::time_point t_start; ///< Internal timer | |
| #endif | |
| f64 nanosec; ///< Time in nanoseconds | |
| private: | |
| #if defined(USE_PLF_TIMER) | |
| plf::nanotimer timer; ///< Internal timer | |
| #else | |
| std::chrono::steady_clock::time_point t_start; ///< Internal timer | |
| #endif | |
| f64 nanosec; ///< Time in nanoseconds | |
| public: |
| f64 nanosec; ///< Time in nanoseconds | ||
|
|
||
| /// Constructor, init nanosec to 0 | ||
| Timer() : nanosec(0.0) {} |
There was a problem hiding this comment.
When using the std::chrono backend, t_start is not initialized in the constructor. If stop() is called before start(), the calculated duration will be relative to the clock's epoch, resulting in a very large and incorrect value. It is safer to initialize t_start to the current time or ensure nanosec remains 0 if the timer hasn't been started.
| Timer() : nanosec(0.0) {} | |
| Timer() : nanosec(0.0) { | |
| #if !defined(USE_PLF_TIMER) | |
| t_start = std::chrono::steady_clock::now(); | |
| #endif | |
| } |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Workflow reportworkflow report corresponding to commit 4e252c0 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportNo relevant changes found. You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review. Doxygen diff with
|
No description provided.