On 08/18/2010 12:21 AM, Jonathan M Davis wrote:
On Tuesday 17 August 2010 21:46:27 SHOO wrote:
2. As a user of this module, I would much prefer to have an exception
throw when I call start or stop function out of order, instead of silent
return. If the only returns, there semantic requirement for having stop
function - it is has then same meaning as peek.
There are three ways:
1. enforce
2. assert
3. return;
Which do you like?
I like #2 because I want to suppress influence to runtime.
I think that the typical thing to do at this point is to use assert internally
to verify the state of the struct or class itself, and to use enforce to check
input from the users of the module.
I agree. Here is a brief code review, I'm basing it on
http://ideone.com/TVw1P:
Line 21: This "struct" trick is interesting, why are you using it? Also,
I recommend type names to start with a capital (not vital because this
is private anyway).
Line 47 and beyond: shouldn't we use at best use ulong instead of long?
Line 94 and others: you have a fair amount of @system methods that are
very @safe. Why mark them as @system? I think the entire module is @safe
in fact.
Line 212: So far you used seconds/fromSeconds, useconds/fromUseconds
etc. Now you have interval() which does not inform about the unit of
measurement. How about exactSeconds or realSeconds? Speaking of which,
I'm not sure if anyone would be ever interested in the truncated
seconds, so why not just dump that guy and then rename interval() to
seconds()? To further clarify: if someone cares only about truncated
seconds, it's unlikely they're also performance-worried enough to not
work with floating-point numbers (besides, heck, FP division is actually
much faster than integral division).
Line 229-240: consolidate these two operators with a mixin.
Line 265-284: same thing
Line 318: return cast(int) (value - t.value);
Line 335-359: consolidate with mixin
Line 376-389: same thing
Line 414: no need for @trusted, casting numbers is not unsafe.
Line 438: Please don't give examples using the scope keyword; it is well
on its way to deprecation.
Line 461: it would be difficult to justify the design choice of making
this as a class. First, it has methods that I don't see how anyone can
override gainfully: objectTime(), reset(), start(), stop(), peek().
These are quite clear pieces of functionality. Why would anyone override
them? Second, if you're measuring exact timings, you're very performance
sensitive. Inserting an object allocation in the middle of everything is
overkill. I think all we need is a simple, cheap stack type similar to
Ticks.
Line 489: if we go with a struct design, we should have a constructor
that "autostarts" a StopWatch, e.g.:
auto myWatch = StopWatch(AutoStart.yes);
Line 637: QueryPerformanceCounter's return value is ignored. Please make
a pass through all code and make sure you check all return values.
Nice work!
Andrei
_______________________________________________
phobos mailing list
[email protected]
http://lists.puremagic.com/mailman/listinfo/phobos
