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

Subject: Re: [boost] [timer] Boost Timer Library Version 2
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2011-09-27 11:42:18

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.

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

 - s/by by/by/

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

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

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

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

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

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

I'd do s/cpu/CPU/ on default_format.  (BTW, it's rather surprising that things at namespace scope declared in timer.hpp are not defined in timer.cpp.)

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;
      }
   }

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?

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.

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

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

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

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer                     using std::disclaimer;
Dev Tools & Components
Susquehanna International Group, LLP  http://www.sig.com

________________________________

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.