GNOME Bugzilla – Bug 98536
Add g_timer_continue()
Last modified: 2011-02-18 16:07:08 UTC
Currently we have g_timer_start() - resets and starts the timer g_timer_stop() - stops the timer g_timer_reset() - resets the timer Adding g_timer_continue() that starts the timer without resetting it would make the presence of reset() seem a whole lot more natural; start() would then just be a shortcut for reset() + continue(). It might be useful in the case where you want to time multiple iterations of something, but have significant overhead between iterations that need to be excluded from the timing. See bug 88768 and the mailing list thread linked to from there.
In case this is still of interest, attached is a tentative patch that implements a g_timer_continue function without introducing new variables to the struct. Basically the patch resets the start time to the current time minus the previously elapsed time, so that g_timer_elapsed still returns the total elapsed time. The patch also includes an addition to testglib. Cheers -Tim
Created attachment 20893 [details] [review] adds g_timer_continue
Created attachment 21875 [details] [review] patch creates g_timer_continue() and adds test case (win32 code is not tested)
Second patch adds 'since 2.4' to the API reference docs. Sorry for the flood.
Basically looks fine,but some patch comments: * The code you have in a couple places: +#ifdef G_OS_WIN32 + timer->end = timer->start; +#else + timer->end.tv_sec = timer->start.tv_sec; + timer->end.tv_usec = timer->start.tv_usec; +#endif can be written simply as: timer->end = timer->start; * I don't actually understand why you are setting timer->end in start/reset. g_timer_continue() without calling g_timer_stop() shouldn't have any defined behavior. * The indentation needs fixing to GTK+/GNU standards. (Just match what's there already. Editor tabs should be 8 characters, which I mention since it looks like there is some chance that you are using 2 character tabs.) * g_timer_continue() needs a comment describing the algorithm of changing the start time. * + g_return_if_fail (timer->end >= timer->start); /* wrapping not supported */ isn't really an appropriate use of g_return_if_fail() - since it isn't the programmers fault. It shouldn't be hard to make the code handle the situation
Created attachment 23618 [details] [review] new patch for g_timer_continue
Thanks for your comments. Attached above a cleaned-up version of the patch that addresses the issues raised. In particular: * timer->end not set anymore in g_timer_start/g_timer_reset (was set originally so g_timer_start could be implemented as 'reset + continue', which is not necessary of course). * indenting and tabs fixed (hopefully) * comment about how the start time is changed added to docs * wrapping is automatically supported and needs neither the assertion nor any special treatment, as far as I can see. Cheers -Tim
Looks good, but a few more comments: > , or the > +behaviour of the timer will be undefined for the rest of its lifetime. Just omit this phrase. As soon as the user violates a "must" in the API docs, we don't need to document what goes wrong. GLib could start printing "fool! fool!" every time the user calls g_free(). +g_timer_continue() works internally by resetting the start time of the +timer to the current time minus the previously elapsed interval. I meant a code comment. The API docs should leave the internals opaque. + g_return_if_fail (timer->active == FALSE); /* should this silently be ignored? */ The g_return_if_fail() is exactly right.
Created attachment 23644 [details] [review] new patch for g_timer_continue
In CVS and glib-2.3.2. Thu Jan 22 13:55:44 2004 Owen Taylor <otaylor@redhat.com> * glib/gtimer.c: Add g_timer_continue(). (#98536, Tim-Philipp Müller)