Again, here is the previous question. I have (re)revised my custom scoped timer using the valuable feedback I got from a helpful member of this community. However I feel the need to ask this question so that my new solution is reviewed.
Here is the code (Compiler Explorer):
#include <chrono>
#include <type_traits>
#include <functional>
#include <exception>
#include <utility>
// these are only required by client code
#include <thread>
#include <cstdio>
#include <fmt/core.h>
#include <fmt/chrono.h>
#define NODISCARD_WARNING_MSG "ignoring returned value of type 'ScopedTimer' might " \
"change the program's behavior since it has side effects"
template < class Duration = std::chrono::microseconds,
class Clock = std::chrono::steady_clock >
requires ( std::chrono::is_clock_v<Clock> &&
requires { std::chrono::time_point<Clock, Duration>{ }; } )
class [[ nodiscard( NODISCARD_WARNING_MSG ) ]] ScopedTimer
{
public:
using clock = Clock;
using duration = Duration;
using rep = Duration::rep;
using period = Duration::period;
using time_point = std::chrono::time_point<Clock, Duration>;
using callback_type = std::conditional_t< noexcept( Clock::now( ) ),
std::move_only_function<void ( Duration ) noexcept>,
std::move_only_function<void ( Duration,
std::exception_ptr ) noexcept> >;
[[ nodiscard( "implicit destruction of temporary object" ) ]] explicit
ScopedTimer( callback_type&& callback = nullptr ) noexcept( noexcept( now( ) ) )
: m_callback { std::move( callback ) }
{
}
[[ nodiscard( "implicit destruction of temporary object" ) ]]
ScopedTimer( ScopedTimer&& rhs ) noexcept = default;
~ScopedTimer( )
{
if ( m_callback == nullptr )
return;
if constexpr ( noexcept( now( ) ) )
{
const time_point end { now( ) };
m_callback( end - m_start );
}
else
{
try
{
const time_point end { now( ) };
m_callback( end - m_start, nullptr );
}
catch ( ... )
{
const std::exception_ptr ex_ptr { std::current_exception( ) };
m_callback( duration { }, ex_ptr );
}
}
}
[[ nodiscard ]] const time_point&
get_start( ) const& noexcept
{
return m_start;
}
[[ nodiscard ]] const time_point&
get_start( ) const&& noexcept = delete;
[[ nodiscard ]] const callback_type&
get_callback( ) const& noexcept
{
return m_callback;
}
[[ nodiscard ]] const callback_type&
get_callback( ) const&& noexcept = delete;
void
set_callback( callback_type&& callback ) & noexcept
{
m_callback = std::move( callback );
}
void
set_callback( callback_type&& callback ) && noexcept = delete;
[[ nodiscard ]] duration
elapsed_time( ) const& noexcept( noexcept( now( ) ) )
{
return now( ) - m_start;
}
[[ nodiscard ]] duration
elapsed_time( ) const&& noexcept( noexcept( now( ) ) ) = delete;
[[ nodiscard ]] time_point static
now( ) noexcept( noexcept( clock::now( ) ) )
{
return std::chrono::time_point_cast<duration>( clock::now( ) );
}
private:
time_point const m_start { now( ) };
callback_type m_callback;
};
template <class Callback>
ScopedTimer( Callback&& ) -> ScopedTimer<>;
template <class Duration>
ScopedTimer( std::move_only_function<void ( Duration ) noexcept> ) -> ScopedTimer<Duration>;
// --------------------------- client code ---------------------------
using std::chrono_literals::operator""ms;
template <class Duration, class Clock>
ScopedTimer<Duration, Clock> func( ScopedTimer<Duration, Clock> timer )
{
std::exception_ptr ex_ptr;
timer.set_callback( [ &ex_ptr ]( const auto duration ) noexcept
{
try
{
fmt::print( stderr, "\nTimer in func took {}\n", duration );
}
catch ( ... )
{
ex_ptr = std::current_exception( );
}
} );
if ( ex_ptr )
{
try
{
std::rethrow_exception( ex_ptr );
}
catch ( const std::exception& ex )
{
fmt::print( stderr, "{}\n", ex.what( ) );
}
}
std::this_thread::sleep_for( 200ms );
return timer;
}
int main( )
{
std::move_only_function<void ( std::chrono::milliseconds ) noexcept> f =
[&]( const auto duration ) noexcept
{
try
{
fmt::print( stderr, "\nTimer took {}\n", duration );
}
catch ( ... ) { }
};
ScopedTimer timer { std::move( f ) };
std::this_thread::sleep_for( 100ms );
ScopedTimer t;
auto t2 = func( std::move( timer ) );
t.set_callback( []( const auto d ) noexcept {
fmt::print( stderr, "\nTimer t took {}\n", d ); } );
std::this_thread::sleep_for( 500ms );
}
Chnages:
- I have added 3
[[nodiscard(message)]]
attributes to the code. Although I'm not sure how the one for move constructor might be useful. No valid scenario comes to my mind. - I also added setters and getters for members
m_start
andm_callback
and also deleted their rvalue reference overloads. - Used the
class
keyword instead ofstruct
and thus made the two member variables private. - Added a second template deduction guide so that the type parameter
Duration
can be deduced from the duration parameter type of thestd::move_only_function
object that is passed to the constructor ofScopedTimer
.
Any suggestions could be valuable.
1 Answer 1
I think you reached all your goals with this class, and you implemented all the suggestions I made, so I don't have much to say about it anymore. You could consider adding some more utility functions. For example, a cancel()
function that removes the callback and returns the (discardable) elapsed time. Or pause()
and continue()
functions. But the YAGNI principle applies here.
The only issue I see with this code is that it looks overengineered, and it now relies on C++23, which means that not a lot of codebases will be able to use it yet.
-
\$\begingroup\$ It's good to hear this all. And no, I'm only going to use this class in one of my open source hobby projects so it's fine that it's relying on C++23 support. \$\endgroup\$digito_evo– digito_evo2023年03月11日 11:21:49 +00:00Commented Mar 11, 2023 at 11:21
-
\$\begingroup\$ And do think that the
cancel()
function could be named asditch_callback()
or maybediscard_callback()
? \$\endgroup\$digito_evo– digito_evo2023年03月11日 12:26:19 +00:00Commented Mar 11, 2023 at 12:26 -
1\$\begingroup\$
unset_callback()
orreset_callback()
might be better names. Although you can always doset_callback(nullptr)
to get the same effect. \$\endgroup\$G. Sliepen– G. Sliepen2023年03月11日 12:29:52 +00:00Commented Mar 11, 2023 at 12:29 -
\$\begingroup\$ Yeah
unset_callback()
sounds better to me. And of course set_callback(nullptr) can do that but it doesn't return the elapsed time so I think it's good to implement the unset_callback. \$\endgroup\$digito_evo– digito_evo2023年03月11日 12:41:25 +00:00Commented Mar 11, 2023 at 12:41 -
1\$\begingroup\$ I don't think it matters: the deduction guide is only used to determine which template parameters to use, once that is known it will call the actual constructor. If your constructor has only one form, then using the same one in the deduction guide is not bad though :) \$\endgroup\$G. Sliepen– G. Sliepen2023年03月12日 21:13:26 +00:00Commented Mar 12, 2023 at 21:13
Explore related questions
See similar questions with these tags.