Re: [boost] Low-level precision timer proposal - Part II

From: Philippe Vaucher (philippe.vaucher_at_[hidden])
Date: 2006-07-18 06:10:56

> I'd also like you and other participants to try answering to points
> marked with "gps" in the source code.
>

------------------------------------------------------------------------------
// gps - temporary helper
void global_throw_helper(const char* msg)
{
    throw std::runtime_error(msg);
}

We'd just use boost::throw_exception here.

------------------------------------------------------------------------------
template <typename T>
struct clock_device_traits
{
    // gps TODO
    //has_synch_start?
};

I think maybe we'd have a clock_device_base that is inherited by every
clock_devices, that would be templated on the time type and typedefs
time_duration_type, so the elapsed timer that accepts a clock_device can
also typedef those.

------------------------------------------------------------------------------
// gps - do we need these two typedefs?
// typedef TimePoint time_type;
// typedef typename Device::time_point_type time_point_type;

I think it's convenient, I even propose we do :

typedef ClockDevice clock_type;
typedef typename ClockDevice::time_type time_type;
typedef typename time_type::time_duration_type time_duration_type;

So the user  can know the clock_type for when he uses a convenience typedef
like "typedef timer<microsec_device> microsec_timer;" which is likely to be
declared in the library.

------------------------------------------------------------------------------
void resume(); // gps - what if you call twice pause, twice resume
consecutively??
// bool is_running() const // gps - add this??
// gps: I see no point in having clear_elapsed()

I think calling twice pause or resume should have no effect. Not doing so is
too error prone imho.
is_running() sounds like good idea tho I can't really think of a use for it
so maybe not if that means having to add a data member to implement it.
I think reset() was good, why don't you see a point for it ? If you want to
time two consequent things individually and use the same object ?
------------------------------------------------------------------------------
template <typename D>
void elapsed_timer<D>::start() // may throw
{
    restart(); // this is just the same as restart();
               // maybe we can come up with a common name -gps
}

I think calling start() twice should not reset it, I think restart should
call reset() then start().
------------------------------------------------------------------------------
template <typename D>
void elapsed_timer<D>::pause()
{
    // throw if(!is_running) ?? - gps
    duration += device.elapsed();
    is_running = false;
}

Again, I think pause should pause if not already paused, otherwise do
nothing, like the implementation I uploaded based on Jeff's design.
------------------------------------------------------------------------------
// Specific Clock Devices
// gps - should this be copyable?

Why not ?

Anyway, I'll skip the other points as it's more implementation detail it
seems, but I'll explain more how I think the timer should behave :

- Calling start() should start if not already started, subsequents calls to
start() do nothing.
- Calling pause() should pause if not already paused, subsequents calls to
pause() do nothing.
- Calling resume() should start the timer again at the time duration the
timer was if not already start, subsequents calls to resume() do nothing.
- Calling reset() should reset the current elapsed time to 0.
- Calling restart() should call reset() then start().
- Calling elapsed() should return the current timer value.

I also think your interface lacks the hability to set the initial time
offset, which could be usefull in some situations.

Ok, it's a start ! I'll see if I can comment on the other gps points in a
near future.

Philippe