Basic scoped timer struct (extensively redesigned)

Question

Here is the original question. I have extensively revised and redesigned my custom scoped timer for my needs using the feedback I got from two helpful members of this community. However I was told to ask a new question in case I want my new solution to be reviewed.

Here is the revised version (live on Godbolt):

#include <chrono>
#include <type_traits>
#include <utility>
#include <functional>

#include <thread>       // only needed for client code
#include <cstdio>       // ditto
#include <fmt/core.h>   // ditto
#include <fmt/chrono.h> // ditto


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>{ }; } )
struct [[ nodiscard ]] ScopedTimer
{
    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::add_pointer_t<void ( Duration ) noexcept>;

    static_assert( std::is_nothrow_invocable_r_v<void, callback_type, duration> );

    time_point const start { now( ) };
    callback_type callback;

    explicit ScopedTimer( callback_type const call_back = nullptr ) noexcept( noexcept( now( ) ) )
        : callback { call_back }
    {
    }

    ScopedTimer( const ScopedTimer& rhs ) = default;

    ScopedTimer( ScopedTimer&& rhs ) noexcept
        : start    { std::move( rhs.start ) },
          callback { std::exchange( rhs.callback, nullptr ) }
    {
    }

    ~ScopedTimer( )
    {
        if ( callback == nullptr )
            return;

        if constexpr ( noexcept( now( ) ) )
        {
            const time_point end { now( ) };
            std::invoke_r<void>( callback, end - start );
        }
        else
        {
            try
            {
                const time_point end { now( ) };
                std::invoke_r<void>( callback, end - start );
            }
            catch ( ... ) { }
        }
    }

    [[ nodiscard ]] duration
    elapsed_time( ) const& noexcept( noexcept( now( ) ) )
    {
        return now( ) - start;
    }

    [[ nodiscard ]] time_point static
    now( ) noexcept( noexcept( clock::now( ) ) )
    {
        return std::chrono::time_point_cast<duration>( clock::now( ) );
    }
};


// --------------------- client code e.g. in Main.cpp -----------------------

using std::chrono_literals::operator""ms;

template <class Duration, class Clock>
ScopedTimer<Duration, Clock> func( ScopedTimer<Duration, Clock> timer )
{
    timer.callback = []( const auto duration ) noexcept
                     {
                        try
                        {
                            fmt::print( stderr, "\nTimer in func took {}\n", duration );
                        }
                        catch ( ... ) { }
                     };

    std::this_thread::sleep_for( 200ms );

    return timer;
}

int main( )
{
    ScopedTimer timer { []( const auto duration ) noexcept
                        {
                            try
                            {
                                fmt::print( stderr, "\nTimer took {}\n", duration );
                            }
                            catch ( ... ) { }
                        } };

    std::this_thread::sleep_for( 100ms );

    ScopedTimer t;

    auto t2 = func( std::move( timer ) ); // timer is now moved-from, so
                                          // its dtor won't invoke its callback

    t.callback = []( const std::chrono::microseconds d ) noexcept {
                  fmt::print( stderr, "\nTimer t took {}\n", d ); };

    std::this_thread::sleep_for( 500ms );
}

Notable changes from the previous solution:

1. ScopedTimer now has a move and a copy constructor that behave as I expect. The move constructor assigns nullptr to the callback member of the moved-from object so whenever its destructor is called, it checks the callback member and if it's nullptr then it realizes that *this has been moved from and that it should not invoke its callback function. With that said, I could not implement the assignment operators apparently because the start member is const so it cannot be reassigned in the assignment operators.
2. It now has several using declarations for the typenames that are used by the struct (similar to classes in std library) so that the client code can have easy access to these typenames if needed.
3. Added a [[nodiscard]] attribute to the struct's declaration so that the compiler gives warnings whenever a ScopedTimer instance returned from a function is ignored.
4. Defined (and thus restricted) the callback_type as a pointer type to a function that takes a duration and return void and must be noexcept.
5. Used a static assertion to make sure that the callback_type is nothrow-invocable. However, I'm not sure whether this assertion is redundant or not.
6. Assigned nullptr (as the default argument) to the call_back parameter of the constructor so that the client code can create an object without passing a callback pointer to the constructor (i.e. default construction is possible).
7. Used std::invoke_r to invoke the callback inside the destructor.
8. Added a non-static member function that retrieves the elapsed time when called which can come in handy for some purposes.
9. The now() function is declared as a static member function since it doesn't access any non-static members of the struct.
10. Because the callback_type requires it, any callback function passed to a ScopedTimer object needs to be marked as noexcept. This requirement exists because the callback will be invoked inside the destructor so it should not throw. This puts the responsibility on the client code to make sure that any exceptions thrown inside the passed callback function are properly handled.

Regarding the above snippet, I have defined a sample function named func that represents how a ScopedTimer object (ScopedTimer timer) can be moved/copied to different functions and that they can return it to their call site. Also it is shown that a ScopedTimer object (ScopedTimer t;) can be default-constructed (i.e. without specifying a callback) and then be assigned a callback at a later stage.

With all that said, three things still bother me:

• Firstly, in case now() throws inside the destructor, it is caught by a catch-all clause and ignored. I could not find an easy way to pass the exception (or maybe its std::exception_ptr) to the call site where the ScopedTimer object was created. Is this really an issue? I guess if done correctly, it could increase the code correctness;
• Secondly, my current implementation of the ScopedTimer::callback_type doesn't allow lambda expressions that have a non-empty capture clause (e.g. [&] or [=]). Those types of capture clauses could be useful for the client code in some ways. But my current code does not compile when those capture clauses are used. I have discovered a new C++23 class template called std::move_only_function which probably could be used as the callback_type (e.g. using callback_type = std::move_only_function<void ( Duration ) const noexcept>;) to solve this issue.
• Thirdly, I would like to prevent prvalue objects from calling elapsed_time() since it will always result in a 0 as a duration being returned (e.g. 0 ms). I mean semantically it does not make much sense if a prvalue object calls that member function. Can this be restricted?

As always, any further suggestions are appreciated.

Answer 1

It's great to see the improvements you made! Almost all of them make sense and improve the quality and usability of the class.

Using std::function to store the callback

Your callback_type is just a plain old pointer-to-function. As you mentioned, that's indeed unfortunate, as this limits what kind of callbacks you can pass. So ideally we want:

using callback_type = std::function<void(Duration)>;

But then your code won't compile any more, which is probably why you went for the function pointer. The big issue is that Duration is a template parameter, and std::function<void(Duration)> is a dependent type, and a lambda does not have the same type as a std::function, so when you pass a lambda to the constructor, the compiler has no idea what to do with it.

A user-defined deduction guide might help here. The easiest is just to add this after the definition of class ScopedTimer:

template<typename T>
ScopedTimer(T) -> ScopedTimer<>;

This will cause a statement like:

ScopedTimer t([](auto duration){…});

to compile, and Duration to have the default value (std::chrono::microseconds). The drawback is that there is no deduction on the type of the parameter of the lambda, so the following won't compile:

ScopedTimer t([](std::chrono::seconds duration){…});

But then again, that didn't seem to work with your version either. Another drawback is that you cannot have a std::function that is noexcept, but see below.

Since you deleted the copy constructor, the only thing you will ever do with callback, besides calling it, is to move from it in the move constructor. So indeed with C++23 you could use std::move_only_function to store the callback, as this will allow even more types of functions to be stored. The drawback of course is that C++23 is not finished yet, and it might take a few more years before compilers and standard libraries have full support for it, but you can use #if __cpp_lib_move_only_function to test for it.

Consider removing noexcept

I think it's not worth making everything noexcept and catching exceptions in the destructor. You should keep it for the move constructor, but that's about it.

Consider that someone might do more in the callback than just print a message. Maybe they want to unlock a mutex, commit a transaction to a database, or something else that is important and should not cause the program to hide failure.

Note that if you want to keep noexcept, you should ask yourself why you require the callback function to be noexcept, but allow Clock::now() to throw. And if you are going to catch all exceptions in the destructor anyway, why not allow the callback to throw, as the exception won't escape the destructor then?

Preventing prvalues from calling elapsed_time()

I would like to prevent prvalue objects from calling elapsed_time() since it will always result in a 0 as a duration being returned (e.g. 0ms).

You can do it by adding a deleted overload with an rvalue ref-qualifier:

void elapsed_time( ) const&& = delete;

Make the member variables private

I would make the member variables private. Especially if you worry about things like prvalue objects calling elapsed_time(), I don't think you want to allow any code to accidentally modify start and/or callback. If you really want to make that possible, consider adding member functions to do such modifications, perhaps like a reset() to reset the timer. This makes things more explicit.

Unnecessary use of std::invoke_r()

The functions std::invoke() and std::invoke_r() are mainly useful if you have no idea what the type of the function object you pass it is, for example in the case of:

template<typename Func, typename Arg>
auto call_with_argument(Func&& func, Arg&& arg) {
    return std::invoke(std::forward<Func>(func), std::forward<Arg>(arg));
}

Because writing return func(arg) might not be valid if func is a pointer-to-member-function or pointer-to-data-member. But you know exactly what the type of callback is: it's a void (*)(Duration) (or std::function<void(Duration)> with my suggested change). So writing:

callback(end - start);

Is guaranteed to be perfectly valid. You are also sure that the return value is void, so no need to cast it to void again using std::invoke_r<void>().

While it's not wrong to use std::invoke_r() here, it is more complicated than necessary, and gives the idea that something special is going on here that necessitates its use.

Answer 2

Thanks to the valuable feedback from G. Sliepen, I came up with yet another slighly improved solution.

Here is my revised version (live on Compiler Explorer):

#include <chrono>
#include <type_traits>
#include <functional>
#include <exception>
#include <utility>

#include <thread>
#include <cstdio>
#include <fmt/core.h>
#include <fmt/chrono.h>


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>{ }; } )
struct [[ nodiscard ]] ScopedTimer
{
    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> >;

    time_point const start { now( ) };
    callback_type callback;

    explicit ScopedTimer( callback_type&& call_back = nullptr ) noexcept( noexcept( now( ) ) )
        : callback { std::move( call_back ) }
    {
    }

    ScopedTimer( ScopedTimer&& rhs ) noexcept
        : start    { std::move( rhs.start ) },
          callback { std::move( rhs.callback ) }
    {
    }

    ~ScopedTimer( )
    {
        if ( callback == nullptr )
            return;

        if constexpr ( noexcept( now( ) ) )
        {
            const time_point end { now( ) };
            callback( end - start );
        }
        else
        {
            try
            {
                const time_point end { now( ) };
                callback( end - start, nullptr );
            }
            catch ( ... )
            {
                const std::exception_ptr ex_ptr { std::current_exception( ) };
                callback( duration { }, ex_ptr );
            }
        }
    }

    [[ nodiscard ]] duration
    elapsed_time( ) const& noexcept( noexcept( now( ) ) )
    {
        return now( ) - 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( ) );
    }
};

template <class Callback>
ScopedTimer( Callback ) -> ScopedTimer<>;


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.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<std::chrono::milliseconds> timer { std::move( f ) };

    std::this_thread::sleep_for( 100ms );

    ScopedTimer t;

    auto t2 = func( std::move( timer ) );

    t.callback = []( const auto d ) noexcept {
                  fmt::print( stderr, "\nTimer t took {}\n", d ); };

    std::this_thread::sleep_for( 500ms );
}

So basically, the client code can now pass in different callback types including lambda expressions that have non-empty capture clauses.

And more importantly, the code now forces the client code to provide a callback type that also has an extra parameter of type std::exception_ptr (only forced when the provided class Clock is non-trivial, e.g. std::chrono::gps_clock) so that whenever a non-trivial clock's now() method throws inside the destructor, the pointer to the caught exception is passed to the callback and the callback is responsible to handle it properly. This way, all the exceptions can be caught and dealt with at the call site (e.g. by using a [&ex_ptr] capture clause in order to copy the exception's pointer to the call site and then do a std::rethrow_exception(ex_ptr) there) as shown in the function func in the above snippet.