[phobos] datetime review

[Andrei Alexandrescu]

On 10/9/10 19:57 CDT, Jonathan M Davis wrote:
> I ended up making it fairly easy to _use_ in generic code, I believe, but it
> doesn't do much to generate code, which does indeed result in a lot of generic
> and repetitive code.

Libraries that contain type names in function names have trouble with 
generic code.

> They were meant primarily to better document what a function took (like Year,
> MonthOfYear, DayOfMonth rather than int, int, int) and they restrict the range
> of what you can feed a function (like short instead of int), but they are rather
> ugly and using anything smaller than int gets a bit annoying to use as I found
> in the unit tests. It does have the advantage of making sure that the programmer
> restricts their values to the smaller size, but since the smaller size still
> isn't the exact range required for the given value (e.g. 1 - 12 for months or 1
> - 31 for days), it doesn't really help all that much anyway. It also creates
> greater confusion when various other functions (like addXXX()) take longs,
> though if I did it correctly, that it least manages to indicate when you use a
> value in a specific range rather than x amount of a time unit (e.g. the exact
> month vs the number of months to add). It certainly isn't the greatest as is
> though, and I'm more than open to fixing it.
>
> The main one that I'm leary on fixing is StdTime (and AdjustedTime) since it has
> a specific meaning beyond long and it helps with the documentation. But ideally,
> programmers would use such functions very sparingly anyway, since SysTime wraps
> and deals with most of that for you.

My stance is simple: any global alias that defines a structured type in 
terms of an unstructured type is undesirable. This is because the alias 
lures the programmer into a false sense of security. So you'd have 
difficulty pushing through "alias Anything long;". If SysTime is always 
long, we may as well use long. If SysTime is system-dependent (a la 
size_t), then the case is stronger.

Getting back to Bounded, I'm thinking of allowing an optional string as 
the last argument:

alias Bounded!(long, long.min, long.max, "StdTime") StdTime;

This creates a type that's pretty much like long except it's distinct 
from any other type, even other types that have long.min and long.max as 
their bounds.

The advantage is that people can't pass an unadorned long to a function 
expecting an StdTime. They'd have to write foo(StdTime(100)). Talk about 
library-defined typedef! :o)

>> The names are self-explanatory, no need to repeat them!
>
> I added the individual enume comments at the last minute as soon as I realized
> that for some reason putting ddoc comments on the enum itself didn't result in
> the enum values being shown in the documentation (highly negative in my opinion

I think it ain't that bad. Put a comment on the first value and then 
"/// ditto" for the others. They will be nicely grouped.

> For Clock, Dur, and IRange, however (which are similar namespaces), I really do
> think that the added namespacing is well worth it. It makes the code much
> clearer. Dur in particular would be bad to have its functions separated out,
> because then you have functions like years(), months(), and days() which are
> going to conflict all over the place with variable  names.

I'm just wondering how come dates and times are the only API that's 
hairy enough to require carving namespaces within the whole Phobos. I'm 
just saying a solid argument should be brought up. Otherwise anyone 
could use this as a precedent to justify unnecessarily baroque APIs.

>> Suggestions: (a) throw TUnitConv away; (b) define this and friends at
>> global scope:
>>
>> long convert(TUnit from, TUnit to)(long);
>
> I'm not sure if convert is descriptive enough (TUnitConv.to!() has the distinct
> advantage of being quite explicit even if it isn't very pretty), but given how
> little is in TUnitConv, that's probably a good idea.

For one thing, "to" is not a good name when you need to specify the type 
of "from" as well.

auto a = to!int("123");

makes sense because you get to see the subject of "to". But

auto a = to!(TUnit.weeks, TUnit.seconds)(123);

does not parse remotely like English, whereas

auto a = convert!(TUnit.weeks, TUnit.seconds)(123);

can be reasonably evoked and remembered as "convert from weeks to seconds".

Finally, you need to put faith in overloading. Even if someone else 
defines some "convert" template, only yours takes two TUnit parameters.

>> That being said, convert() does go against the grain of elaborate typing
>> for durations. I'd accept it as a low-level function, but really the
>> library currently fosters elaborate types for durations... so there's a
>> disconnect here.
>
> The library itself needs it regardless. As for user code, well sometimes they
> may very well need to deal with converting seconds to minutes and the like, so I
> think that it's worthwhile to have in the API, but it's not something that I'd
> expect to be heavily used.

Fine.

>> * As another random example:
>>
>> DayOfWeek getDayOfWeek(DayOfGregorianCal day) pure nothrow
>>
>> could/should be (in client use):
>>
>> get!(TUnit.dayOfWeek, TUnit.dayOfGregorianCal)(day);
>
> Since those functions would all have to be have specialized version of such
> templates, I'm not sure how much you gain with that approach - particularly when
> there aren't very many of those functions (and IIIRC they're all package or
> private rather than in the API). But it may be worth doing something like that.
> I'm more skeptical of this suggestion than the others though.

What you gain is that the client only needs to remember "get" and the 
types, instead of the cross product of types. Not to mention advantages 
in generic code, although I don't have an example handy.

Fewer functions for a given functionality == better library.

>> * On to duration.d. It's simple: there's no need for three durations.
>> Rename JointDuration to Duration and throw everything else. I'd find
>> myself hard pressed to produce situations in which efficiency
>> considerations make it impossible to cope with a 128-bit representation.
>>
>> * If you do want to go fine-grained durations without the aggravation,
>> use templates. Then you can have Duration!HNSeconds, Duration!Seconds,
>> Duration!Months, whatever you want, without inflicting pain on yourself
>> and others. If you play your cards right, such durations would match
>> 100% the templates you define in core.d and elsewhere so you don't need
>> to make a lot of implementation effort. BUT again I think one duration
>> is enough. Maximum two - short duration and duration.
>
> I really don't think that this is going to work, and the problem is months and
> years (they frequently seem to be the problem) due to the fact that they don't
> have a uniform number of days. Originally, I had a Duration struct templated on
> TUnit, but it resulted in several problems - the biggest having to do with
> addition/subtraction.

Maybe we can throw month durations and year durations away and dedicate 
a few specialized functions to them? For example, given a date and a 
uniform duration, we can add the duration to the year to figure the end 
year etc.

> You can't convert months or years to units smaller than months, and you can't
> convert anything smaller than a month to months or years without a specific date.

Yah, so that fuels my argument that month durations don't make sense. 
They are exceptional so maybe we should just give up integrating then 
into a uniform concept.

> So, you can't add a Duration!month and Duration!day no matter how it may seem
> like you should be able to. Many durationos _could_ be added - such as
> Duration!day and Duration!hour - but the variable sizes of months and years
> poses a big problem for that. To fix the problem, you need a duration which joins
> the two - hence JointDuration. That way, you can write user code like
> Dur.years(7) + Dur.days(2) and have it work. The way I'd originally done that
> resulted in a combinatorial explosion of joint duration types (it was effctively
> templated on the types of duraton that you'd added together to create it), so I
> just combined the months+ and sub-months durations into two types. It cuts down
> on the amount of generated code at least.

I see. How much would it hurt us to drop months and years durations, as 
well as methods such as years() in any duration?


Andrei