On Tue, 17 Aug 2010 19:43:41 +0900, SHOO wrote:

> Here is the module to suggest this time:
>     http://ideone.com/TVw1P

Very nicely coded. I think much better than the messing std.perf. Few things...

1. I didn't found initialization of APPORIGIN. Is it meant to be initialized in runtime, or else module constructor comes to my mind...

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.

3. in std.perf the mili and microseconds use some special casing for computation. I suppose it is to increase floating point accuracy. Does this casing applies to your module and can it be used to increase accuracy ?

and some much less important things...

4. I personally would prefer names like sec, usec, msec instead the longer you use.

5.  m_TimeObject is not an instance of any class. Better name could be probably "m_TimeInit" "m_TimeInstanceInit"

6. Wouldn't be better to name the variable "m_FlagObserving" or "m_FlagStarted" ?

7. ovserving is misspelled.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20100817/471ec5f3/attachment.html>

(2010/08/17 22:39), Michal Minich wrote:
> On Tue, 17 Aug 2010 19:43:41 +0900, SHOO wrote:
>
>  > Here is the module to suggest this time:
>  > http://ideone.com/TVw1P
>
> Very nicely coded. I think much better than the messing std.perf. Few things...
>
> 1. I didn't found initialization of APPORIGIN. Is it meant to be initialized in runtime, or else module constructor comes to my mind...
>

Oh... I'll fix it.

> 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.

> 3. in std.perf the mili and microseconds use some special casing for computation. I suppose it is to increase floating point accuracy. Does this casing applies to your module and can it be used to increase accuracy ?
>

Because there is the issue of license, I don't look std.perf either.
I thought about it just a bit, too, but stopped it because somebody felt
like researching it.
I suppose that it may change a method of the division dynamically.
However, I do not reach it to find a concrete method.

> and some much less important things...
>
> 4. I personally would prefer names like sec, usec, msec instead the longer you use.
>

About it, I intend to prepare for alias.

> 5.  m_TimeObject is not an instance of any class. Better name could be probably "m_TimeInit" "m_TimeInstanceInit"
>
> 6. Wouldn't be better to name the variable "m_FlagObserving" or "m_FlagStarted" ?
>
> 7. ovserving is misspelled.
>

Thanks for your advice! I'll consider your opinion.

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. As for calling stop() out of order, that's a bit harder to say. It's not checking input per se, but it's not exactly checking the internal consistency of the struct either...

I think that I'm going to have to go with enforce() on this one simply because odds are that the users of the module are going to be using the phobos lib which will likely have been compiled in release mode where there are no assertions. As such, an assert would generally be useless, and you're just going to get silent failures. If it were primarily other Phobos modules using it that would be one thing, but since it's definitely intended for general use, I'd have to say use enforce().

Now, what you can do if you think that that could be a performance issue for a high-precision timer is you can have two versions of the function - one which has enforce() and one which silently fails on a mismatch. That one, the user can decide which to use. So, maybe stop() and checkedStop() or something similar.

- Jonathan M Davis

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

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

On Wednesday, August 18, 2010 10:21:56 SHOO wrote:
> > Line 414: no need for @trusted, casting numbers is not unsafe.
> 
> Error: cast from const(long) to real not allowed in safe code

Maybe to!long will work.

> 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

According to TDPL, it's because you have to know the struct's init value at compile time. You can only have one init, and it's quite possible to create default constructors which result in different values even though they don't have any parameters. Classes avoid the problem because their reference-based and the reference itself is what gets the init. But since structs go on the stack, it doesn't work to have an arbitrary default constructor. It might work at some point to have default constructors with certain restrictions (like maybe purity), but for the moment, we don't have a way to make it work.

- Jonathan M Davis

(2010/08/19 2:38), Jonathan M Davis wrote:
> On Wednesday, August 18, 2010 10:21:56 SHOO wrote:
>>> Line 414: no need for @trusted, casting numbers is not unsafe.
>>
>> Error: cast from const(long) to real not allowed in safe code
>
> Maybe to!long will work.
>

But, to!real(long) is system function unfortunately.

>> 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
>
> According to TDPL, it's because you have to know the struct's init value at compile time. You can only have one init, and it's quite possible to create default constructors which result in different values even though they don't have any parameters. Classes avoid the problem because their reference-based and the reference itself is what gets the init. But since structs go on the stack, it doesn't work to have an arbitrary default constructor. It might work at some point to have default constructors with certain restrictions (like maybe purity), but for the moment, we don't have a way to make it work.
>
> - Jonathan M Davis

What is the reason why all types must have .init property? I think that structs may not need to have .init property.

... However, I think that this is a very sensitive problem ATST.

In the first place, should we use class keyword for the classes(of OOP)?
Although there is scope class which seems to satisfy these demands, it
does not function well.
Isn't improving scope more strongly better than marking scope deprecated?

On 08/18/2010 12:21 PM, SHOO wrote:
> (2010/08/18 20:18), Andrei Alexandrescu wrote:
>> 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.

Their implementation is still under flux, but until then, we can't use them as a matter of preference. You may want to eliminate them entirely (or comment them out for now), or try to compile the entire module with @safe: at the top and report every compiler error as a bug. In particular, explicit casting from real to long is entirely safe and should not be an error.

> How about:
> T toSeconds(T=real)() if (isNumeric!T) {
> return (cast(T)value)/TICKSPERSEC;
> }
> @property alias toSeconds!real seconds;

I think that should work.

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

Ha, thanks for the lesson.

>> Line 414: no need for @trusted, casting numbers is not unsafe.
>>
>
> Error: cast from const(long) to real not allowed in safe code

Compiler bug. I wonder whether const has anything to do with (shouldn't).

> 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.

Oh, I missed that the class is final. Thanks. The sad reality is that classes will end up using dynamic allocation, which would cost much more than inserting one check in each of your four primitives.

> 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.

I would like it too.

> 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

Unfortunately that won't help this case because we can't call system APIs during compilation.

> Cannot these be settled by making templates such as hasDefaultConstructor!T and canMakeStaticVariable!T ?

It's possible, but then we'd have structs impossible to clear. Alternatively, we should accept that cleared structs only have the .init values set for their members.

>> 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.

Leaving aside all struct-related issues, I really think it's entirely reasonable to have the default constructor of a stopwatch leave it in the "not started" state. The C++ stopwatches we use at Facebook (which I think are inspired by Boost) need to be explicitly initialized like this:

Timer timer;
timer.start();

or like this:

Timer timer(true);

I didn't hear many protests about that setup. So why not do the same? By default there's no system call, so you can create an array of Stopwatch objects without incurring a large cost. Then you start them manually as you find fit.

> One question: Is there the reason using enum? Should not it be bool?
> auto myWatch = StopWatch(true);

Ha! Good question. Many people consider passing options as Booleans a poor design (the design of Facebook's Timer above notwithstanding :o)). This is because most of the time the reader must look up the manual to see what the meaning of "true" or "false" is. Just search std/ for the string ".yes" and you'll see for example that I don't use true and false for case sensitive, I use CaseSensitive.no or CaseSensitive.yes. It would be really difficult for a reader to figure what indexOf(str1, str2, true) is, but indexOf(str1, str2, CaseSensitive.yes) is very easy to understand.


Andrei

On 08/19/2010 08:38 AM, SHOO wrote:
> (2010/08/19 2:38), Jonathan M Davis wrote:
>> On Wednesday, August 18, 2010 10:21:56 SHOO wrote:
>>>> Line 414: no need for @trusted, casting numbers is not unsafe.
>>>
>>> Error: cast from const(long) to real not allowed in safe code
>>
>> Maybe to!long will work.
>>
>
> But, to!real(long) is system function unfortunately.

I think we should change that to @trusted and file a bug that explicit cast from real to long is @safe.

> What is the reason why all types must have .init property? I think that structs may not need to have .init property.

The reason is that important primitives such as swap() and move() must make sure there exists an "empty" state for all values.

> ... However, I think that this is a very sensitive problem ATST.
>
> In the first place, should we use class keyword for the classes(of OOP)?
> Although there is scope class which seems to satisfy these demands, it
> does not function well.
> Isn't improving scope more strongly better than marking scope deprecated?

I agree. The problem is that improving scope in any interesting way is next to impossible without interprocedural analysis, which has many disadvantages. Therefore I think scope has no chance but deprecation, and that we definitely shouldn't design library elements that require or recommend it.


Andrei

(2010/08/20 0:29), Andrei Alexandrescu wrote:
> On 08/19/2010 08:38 AM, SHOO wrote:
>> (2010/08/19 2:38), Jonathan M Davis wrote:
>>> On Wednesday, August 18, 2010 10:21:56 SHOO wrote:
>>>>> Line 414: no need for @trusted, casting numbers is not unsafe.
>>>>
>>>> Error: cast from const(long) to real not allowed in safe code
>>>
>>> Maybe to!long will work.
>>>
>>
>> But, to!real(long) is system function unfortunately.
>
> I think we should change that to @trusted and file a bug that explicit cast from real to long is @safe.
>

I pray for being cured fast.

>> What is the reason why all types must have .init property? I think that structs may not need to have .init property.
>
> The reason is that important primitives such as swap() and move() must
> make sure there exists an "empty" state for all values.
>
>> ... However, I think that this is a very sensitive problem ATST.
>>
>> In the first place, should we use class keyword for the classes(of OOP)?
>> Although there is scope class which seems to satisfy these demands, it
>> does not function well.
>> Isn't improving scope more strongly better than marking scope deprecated?
>
> I agree. The problem is that improving scope in any interesting way is next to impossible without interprocedural analysis, which has many disadvantages. Therefore I think scope has no chance but deprecation, and that we definitely shouldn't design library elements that require or recommend it.
>
>
> Andrei

It is a destructive change to deprecated scope and needs mass code
renewal. What of present scope are you dissatisfied with? The evolution
that expands present scope if possible is the best.
scope pays the sacrifice and what does get?

I want the page that summarize function/features/modules and reasons examining the abolition.