(2010/08/18 20:18), Andrei Alexandrescu wrote:
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:
Thanks for your detailed review!
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).
I restrict a thing depending on a system in a mass in a namespace "os".
Because I discovered it accidentally, I use the way of this struct in
for fun.
Line 47 and beyond: shouldn't we use at best use ulong instead of long?
I suppose that Ticks takes minus number value.
And, QueryPerformanceCounter seems to give back an LARGE_INTEGER(64bit
signed integer) at least.
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.
The reason is because it does a calculation to lose precision.
I do not yet understand it about @safe/@trusted/@system well.
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).
The beginning was only an interval function.
Because there was suggestion to add seconds/milliseconds/microseconds
to, I implemented them.
For me, I do not think that they are necessary.
How about:
T toSeconds(T=real)() if (isNumeric!T) {
return (cast(T)value)/TICKSPERSEC;
}
@property alias toSeconds!real seconds;
Line 229-240: consolidate these two operators with a mixin.
Line 265-284: same thing
OK, I'll try it.
Line 318: return cast(int) (value - t.value);
No. value is long, it cannot cast to int.
Check this:
import std.stdio;
void main()
{
long l1 = 0x1000000000000001L;
long l2 = 0x4000000000000000L;
writeln(l1); // 1152921504606846977
writeln(l2); // 4611686018427387904
writeln(l1 - l2); // -3458764513820540927
writeln(cast(int)(l1 - l2)); // 1 <- !!!!
}
Line 335-359: consolidate with mixin
Line 376-389: same thing
OK, I'll try it.
Line 414: no need for @trusted, casting numbers is not unsafe.
Error: cast from const(long) to real not allowed in safe code
Line 438: Please don't give examples using the scope keyword; it is well
on its way to deprecation.
Really!? Oh...
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.
The struct does not have a default constructer. This class must
initialize it by all means in runtime. Because this class is a final
class, nothing has override functions. This means that it can show the
best performance by optimization.
There are not advantages with using struct that sacrifice the secure
initialization.
It does not have a meaning to make a factory function for this.
It can be made the variable that is not initialized even if I had
factory function.
auto makeStruct() {
struct StructTmp {
private bool isInitialize = false;
void func() { assert(isInitialize); }
}
StructTmp s;
s.isInitialize = true;
return s;
}
void main() {
auto s1 = makeStruct();
typeof(s1) s2;
s2.func(); // Assertion failure
}
May not such a usage happen in generic programming?
I think that the design of the impossible misuse is better.
If there is even a default constructer, I will use a struct.
I discussed even an argument of before, I really want the default
constructer for struct.
It's my understanding that there are two reasons why a struct doesn't
have a default constructer toward.
Primarily, it can define as a variable without minding an initialization
method in particular generic programming.
Second, it ensure the initialization that resource assignment does not
occur.
See also: http://j.mp/9KGTFd
Cannot these be settled by making templates such as
hasDefaultConstructor!T and canMakeStaticVariable!T ?
In addition, I think that what can define a default constructer is worth
sacrificing these.
Or I think that class is necessary when initialization is necessary.
The problem is a point that class is reference type and need new by all
means. In safeD, I think that there is not the merit that is reference type.
Hmm... Otherwise, I can make it a struct if I delete objectTime as
compromise plan. But there are few merits.
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);
I examine.
One question: Is there the reason using enum? Should not it be bool?
auto myWatch = StopWatch(true);
Line 637: QueryPerformanceCounter's return value is ignored. Please make
a pass through all code and make sure you check all return values.
Ok, I'll fix.
Nice work!
Andrei
Thanks!
_______________________________________________
phobos mailing list
[email protected]
http://lists.puremagic.com/mailman/listinfo/phobos
