[phobos] Proposal of StopWatch module

[SHOO]

(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!