On Tue, Sep 27, 2011 at 11:42 AM, Stewart, Robert <Robert.Stewart@sig.com> 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/