Re: [boost] [timer] Boost Timer Library Version 2

Subject: Re: [boost] [timer] Boost Timer Library Version 2
From: Beman Dawes (bdawes_at_[hidden])
Date: 2011-09-28 12:08:37

On Tue, Sep 27, 2011 at 11:42 AM, Stewart, Robert
<Robert.Stewart_at_[hidden]> wrote:
> Beman Dawes wrote:
>>
>> The docs can be viewed at http://beman.github.com/timer/
>
> The Timer Home page should include an introduction and motivation before describing the two interfaces.

Done.

> CPU Timers page:
>
>  - create_checkpoint() is confusing in the cpu_timer example
>   because "checkpoint" appears so many different times,
>   including in "last_checkpoint" and "checkpoint_timer".  I
>   initially thought that create_checkpoint() was a cpu_timer
>   operation rather than the activity being controlled by the
>   timer.  I think it would be sufficient to change the
>   introductory paragraph to, "The following code calls
>   create_checkpoint() every 20 seconds of user CPU time:".
>   (Note s/twenty/20/ and s/cpu/CPU/ in my version.)

Changed.

>  - s/by by/by/

Fixed.

>  - The Returns and Postconditions for various member functions
>   are written in terms of expositional data members.  You should
>   add "as if" to those definitions or rephrase them to avoid
>   mention of those data members.

Done.

>  - I realize that the member functions are not very complicated
>   and you have some future hope to see this code standardized,
>   but the specification-style documentation is harder to follow
>   than necessary for Boost documentation.  For example, resume()
>   should be documented as, "Starts the timer again, if currently
>   stopped, as if it had been started when it was last stopped.
>   That is, future calls to elapsed() will include the time it
>   was stopped."  The description for start() should note that
>   the elapsed time is reset.

I've done another pass through the reference section, making both
corrections and clarifications. Where the resulting spec still seemed
likely to baffle those not used to the style of the standard library,
I've added a non-normative "Overview:" element.

>  - Why aren't you referring to default_format in the Effects
>   clause for auto_cpu_timer::report()?

The default is supplied at construction. I've changed to a more robust
implementation.

>  - There's no information on thread safety.  stop(), for example
>   is not reentrant.

OK, paragraph added.

>  - There's no information on Boost.Timers overhead relative to
>   timing the target code.

I've added a "Timer accuracy" section to the docs. I'd welcome a patch
to test/cpu_timer_info.cpp that actually measured overhead.

>
>> The code is available at https://github.com/Beman/timer
>
> I'm not sure the behavior of ~auto_cpu_timer() is correct.  Shouldn't the times be reported if the timer was stopped but report() wasn't called?  IOW, I'd have expected a "reported" flag that would control whether to try calling report().

I need to give that a bit of thought...

>
> I would not expect stop() to be a side effect of auto_cpu_timer::report().  I understand its usefulness for correct reporting, but I'd have expected this implementation:
>
>   std::string const & fmt(m_format.empty()
>      ? default_format
>      : m_format);
>   m_os << timer::format(stop(), fmt, m_places);
>   resume();

Good idea! Done.

> I'd do s/cpu/CPU/ on default_format.

Done.

  (BTW, it's rather surprising that things at namespace scope declared
in timer.hpp are not defined in timer.cpp.)

That's been fixed.

> auto_timers.cpp:94 assert(0) is unhelpful.  The assertion should at least be:
>
>   assert(!"Internal Error: Unexpected format character");
>
> However, even better would be the following which accounts for "%%" producing "%" and "%x" producing "%x" for any "x" not in "wustp".  The latter in the output would be a clue to the user that it was not a recognized conversion specification.
>
>   if (*format != '%')
>   {
>      os << *format;
>   }
>   else if (*(format + 1))
>   {
>      ++format;
>      switch (*format)
>      {
>         case 'w':
>         // and so on for u, s, t, and p
>         break;
>         case '%':
>            os << '%';
>         break;
>         default:
>            os << '%' << *format;
>         break;
>      }
>   }

The default and assert has been removed. KISS.

> In cpu_timer.cpp::get_cpu_times(), on Windows, why do you throw away the perfectly good wall clock time when you can't get the process times?

Good catch! That was residue from the original implementation and
should have been changed at the switch to use Boost Chrono. Fixed.

> cpu_timer.cpp::tick_factor() is a non-trivial function, yet cpu_timer.cpp::get_cpu_times() calls it thrice in a row in the non-Windows code.

Actually, three times. Fixed.

> Why is error_code.cpp included in Boost.Timer?  Shouldn't you just use Boost.System?

Development artifact. Removed.

> error_code.cpp:71: s/Sundo/Sun do/

Fixed in Boost trunk.

>> Special thanks to Rob Stewart - many of his suggestions have
>> been incorporated.
>
> Thanks.  Shouldn't some of those use cases be documented with examples to illustrate the flexibility of the API?

Sure! Care to submit something?

Actually, you've already contributed so much that I'd be happy to list
you as a co-author!

--Beman

PS: docs updated at http://beman.github.com/timer/