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

From: Gennaro Prota (gennaro_prota_at_[hidden])
Date: 2006-07-18 08:30:23

On Tue, 18 Jul 2006 12:10:56 +0200, "Philippe Vaucher"
<philippe.vaucher_at_[hidden]> wrote:

>> 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.

I'm afraid I didn't make my point clear. It wasn't about supporting
BOOST_NO_EXCEPTIONS (which I'd like not to --it would require in any
case a re-examination of the code, as I didn't design it with that in
mind). It was to discuss what type of exception(s) we want to throw
and what additional info, if any, we want to add to exception objects.
Maybe Boost.Date-Time already has a well thought-out solution.

>------------------------------------------------------------------------------
>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.

I'm not sure we are understanding each other. The main advantage of my
solution is that it doesn't logically imply/require any usage of a
"time point". That means: if you measure while someone (or some
program/service) adjusts the system clock, or a DST switch happens,
you are *not* affected. Example:

 you begin measuring at 2am of your local time, in the day
 when DST is switched off. Your test takes 1h 30m. With your
 calculation you get:

 start =  2am,  elapsed = 0
 end   = *2:30am*, elapsed = 30 min
 

Your approach would work if date_time offered a concept of time-zero
point: a special time point value which would only be used to
calculate differences, so to elide it automatically:

 (stop-t0) - (start - t0) = stop-start

Such a value could be used as implementation detail. But frankly I
don't see a reason for it from a logical point of view. We only care
about durations and durations are addable; that's all we need.

>------------------------------------------------------------------------------
>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.

Maybe.

>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.

Basically you are saying you are in doubt. I was too, hence my
question (incidentally: what's your concern with an additional data
member??).

>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 ?

restart()?

>------------------------------------------------------------------------------
>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().

Honestly, did you really look at my code? Looks like you keep thinking
in terms of yours. I had no concept of reset() at all.

>------------------------------------------------------------------------------
>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.

I see. But the point is: if the user code happens to call pause()
twice without intervening restarts, is that a sign of a logical error?
It's similar to set a pointer to null after a delete, to ensure a
double deletion is harmless. It may make you feel safer, but in
practice it hides programming errors by eating their immediate ill
effects.

I may see a use for is_running at least in debug mode here.

>------------------------------------------------------------------------------
>// Specific Clock Devices
>// gps - should this be copyable?
>
>Why not ?

It's a choice, and I annotated it to make sure I gave it another
thought at the end of the work.

>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.

Same comment as above.

>- Calling pause() should pause if not already paused, subsequents calls to
>pause() do nothing.

Again.

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

Care to explain?

>Ok, it's a start !

Yes, thanks. At least it should clear some manifest misunderstanding.

--
[ Gennaro Prota, C++ developer for hire ]
[    resume:  available on request      ]