Apologies Yao,

I'm fairly busy this week, but I have it on my agenda to look closely at your work.

Could other Phobos members chime in with reviews? That would be great, thanks.

Andrei

On 8/24/10 20:32 PDT, Yao G. wrote:
> 2010/8/24 SHOO <zan77137 at nifty.com>:
>
>> [snip]
>>
>> In particular, std.gregorian seem to be still insufficient about
>> functions
>> and designs.
>> I think waiting for other modules to mature then to unify is good.
>>
>> [snip]
>
> I did an almost complete implementation of std.gregorian and posted about this on the news group. But it seems that nobody was interested :D
>
> Here's the code: http://ideone.com/uyuZ9
>
> And the original post: http://www.digitalmars.com/pnews/read.php?server=news.digitalmars.com&group=digitalmars.D&artnum=115980
>
>
> Obviously the code can be improved. I was in a haste (I needed this for a small project I'm working on). But maybe something from this implementation can be used to improve the original gregorian.
>
>
> --
> Yao G.
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

On Tue, 24 Aug 2010 12:32:21 -0500, Andrei Alexandrescu <andrei at erdani.com> wrote:

> Apologies Yao,
>
> I'm fairly busy this week, but I have it on my agenda to look closely at your work.
>
> Could other Phobos members chime in with reviews? That would be great, thanks.
>
> Andrei
>

Yeah. Don't worry about that. Is not like I did that module just in the hopes of being integrated or used on Phobos. Like I said, I use it on my on projects. It's just that as Shoo commented about the current state of the official gregorian.d module, I offered my own work, because I think that it could easy the burden of implementing the former.

By the way, now I need time manipulation facilities, so I'm working on the port of the Posix Time classes: http://www.boost.org/doc/libs/1_42_0/doc/html/date_time/posix_time.html

I'll post here the code when I finish it.


-- 
Yao G.

2010/8/24 SHOO <zan77137 at nifty.com>:

> [snip]
>
> In particular, std.gregorian seem to be still insufficient about
> functions
> and designs.
> I think waiting for other modules to mature then to unify is good.
>
> [snip]

I did an almost complete implementation of std.gregorian and posted about this on the news group. But it seems that nobody was interested  :D

Here's the code: http://ideone.com/uyuZ9

And the original post: http://www.digitalmars.com/pnews/read.php?server=news.digitalmars.com&group=digitalmars.D&artnum=115980

Obviously the code can be improved. I was in a haste (I needed this for a small project I'm working on). But maybe something from this implementation can be used to improve the original gregorian.


--
Yao G.

I've had the chance to look at your date lib now, and (without being anywhere near an expert on the matter) I think it seems rather good.


Some comments:


1. I'm not sure I agree that DayOfWeek should be a named enum. Personally, I'd prefer if the weekdays were enumerated like the months,

  alias int DayOfWeek;
  enum : DayOfWeek { Monday, Tuesday, ... }

That way you can get rid of the DOW alias.  In general, I'm not fond of aliases that are just abbreviations, so I suggest removing WeekNo as well. ;)


2. The month enum should have the Month type, I guess?

  enum : Month { January, February, ... }


3. I found it rather strange that the doc comment says that DatePeriod.last() returns the day *after* the period, while DatePeriod.end() is the last day of the period.  I checked boost::date_time, and indeed it seems you've switched the two -- at least in the documentation.


4. The doc comments need a bit of cleaning up.  Examples:
  - There should be some ///ditto comments for the Month,
    Day, and DayOfYear types.
  - The docs for DateRange.this() refer to WeekRange.
  - The docs for Date.toIso8601() mention IsoFormat.Normal,
    rather than IsoFormat.Basic.


5. Is there any reason why you define the trivial opEquals() for DateCount?  I think the compiler takes care of that for you.


I'm looking forward to seeing the time stuff as well!

-Lars


On Tue, 2010-08-24 at 22:32 -0500, Yao G. wrote:
> 2010/8/24 SHOO <zan77137 at nifty.com>:
> 
> > [snip]
> >
> > In particular, std.gregorian seem to be still insufficient about
> > functions
> > and designs.
> > I think waiting for other modules to mature then to unify is good.
> >
> > [snip]
> 
> I did an almost complete implementation of std.gregorian and posted about this on the news group. But it seems that nobody was interested  :D
> 
> Here's the code: http://ideone.com/uyuZ9
> 
> And the original post: http://www.digitalmars.com/pnews/read.php?server=news.digitalmars.com&group=digitalmars.D&artnum=115980
> 
> Obviously the code can be improved. I was in a haste (I needed this for a small project I'm working on). But maybe something from this implementation can be used to improve the original gregorian.
> 
> 
> --
> Yao G.
> _______________________________________________
> phobos mailing list
> phobos at puremagic.com
> http://lists.puremagic.com/mailman/listinfo/phobos

On Wed, 25 Aug 2010 11:34:10 -0500, Lars Tandle Kyllingstad <lars at kyllingen.net> wrote:

> I've had the chance to look at your date lib now, and (without being anywhere near an expert on the matter) I think it seems rather good.

Thanks!

> 1. I'm not sure I agree that DayOfWeek should be a named enum. Personally, I'd prefer if the weekdays were enumerated like the months,
>
>   alias int DayOfWeek;
>   enum : DayOfWeek { Monday, Tuesday, ... }
>
> That way you can get rid of the DOW alias.  In general, I'm not fond of aliases that are just abbreviations, so I suggest removing WeekNo as well. ;)

TBH, I dislike anonymous enums. They tend to polute the scope where they are imported, and sometimes the value name is too ambiguous without the enum name prepended. But in this case, I think the week days are self-describing enough. In fact the enum with the month names also started as named enum, but after writing MonthName.September for the tenth time, I got tired and changed it. Yes, this is a good idea to make this change.

> 2. The month enum should have the Month type, I guess?
>
>   enum : Month { January, February, ... }

Well, it kinda get complicated. Originally, the Month alias was going to be a full fledged structure, but then the things started to get unwieldy because as the month can also be represented with a number (or an enum), I would had to write a lot of operator overloads (for mathematical operations and comparison with ints). So at the end I settled with an integer. But if there's the need to revert back to the Month struct, then the enum wouldn't make sense (an enum of structs?). Short version: Month can change of type so that's why the enum shouldn't be of the aforementioned type.

>
> 3. I found it rather strange that the doc comment says that DatePeriod.last() returns the day *after* the period, while DatePeriod.end() is the last day of the period.  I checked boost::date_time, and indeed it seems you've switched the two -- at least in the documentation.

Yes. Those are some silly mistakes I did and I'll correct them pronto.

> 4. The doc comments need a bit of cleaning up.  Examples:
>   - There should be some ///ditto comments for the Month,
>     Day, and DayOfYear types.

Yes. I'll do that.

>   - The docs for DateRange.this() refer to WeekRange.

Yes, that's one of the perils of copy/paste :( I copied the definition of WeekRange and refactored it to GenericRange but I forgot to update the docs.

>   - The docs for Date.toIso8601() mention IsoFormat.Normal,
>     rather than IsoFormat.Basic.

Oops. An oversight. Originally I named the IsoFormat values as Normal and Extended, but the ISO-8601 spec uses the terms Basic and Extended. I forgot to update the docs.

> 5. Is there any reason why you define the trivial opEquals() for DateCount?  I think the compiler takes care of that for you.

Unfortunately, if I remove the trivial opEquals (the one with the non-const parameter) the compiler complains when I compare a const DayCount with a non-cons one.

> I'm looking forward to seeing the time stuff as well!
>
> -Lars

Thanks again for the time you dedicated to do this review. I have set up a Bitbucket repo with this code at: http://bitbucket.org/gomez/yao-library/src

So whenever I do a change you can go there and review the modifications.

Cheers.

-- 
Yao G.

This has been a busy week, so I didn't have much time to work on the date time modules I'm porting. But I started the work on one of the Time components: TimeDuration. Here's the code:

http://ideone.com/3yNG6

It still needs more polish, better docs and more comprehensive unit test. But it works. The main issue it could have is that it needs another internal module to work, because the TimeDuration struct supports special values (+infinite, -infinite, NAN, etc.), and to add support for those, some helper structs had to be ported. Here's the internal, impl module in question:

http://ideone.com/g9IWA

On the other hand, I did a quick dive on Johnathan code, and I think it's superb. I think it deserves a review, because it could be a better candidate to inclusion that mine. I'll continue porting the Boost date time library, because as I'm using it on my own code, but obviously the better one should be picked.


--
Yao G.

(2010/08/28 17:24), Yao G. wrote:
> This has been a busy week, so I didn't have much time to work on the date time modules I'm porting. But I started the work on one of the Time components: TimeDuration. Here's the code:
>
> http://ideone.com/3yNG6
>
> It still needs more polish, better docs and more comprehensive unit test. But it works. The main issue it could have is that it needs another internal module to work, because the TimeDuration struct supports special values (+infinite, -infinite, NAN, etc.), and to add support for those, some helper structs had to be ported. Here's the internal, impl module in question:
>
> http://ideone.com/g9IWA
>
> On the other hand, I did a quick dive on Johnathan code, and I think it's superb. I think it deserves a review, because it could be a better candidate to inclusion that mine. I'll continue porting the Boost date time library, because as I'm using it on my own code, but obviously the better one should be picked.
>
>
> --
> Yao G.

It looks nice!
I look forward to a std.time included Phobos.

Good luck!

On Sat, 2010-08-28 at 03:24 -0500, Yao G. wrote:
> I did a quick dive on Johnathan code, and I think it's superb. I think it deserves a review, because it could be a better candidate to inclusion that mine. I'll continue porting the Boost date time library, because as I'm using it on my own code, but obviously the better one should be picked.

I haven't had time to really test Jonathan's code, but I've looked at it and I agree it looks very good.  Which one is the better of Yao's and Jonathan's, I cannot say.  They both have their strengths, and neither is complete yet.

Perhaps we should ask for a vote on the NG?

-Lars

On Sat, Aug 28, 2010 at 16:21, Lars Tandle Kyllingstad <lars at kyllingen.net>wrote:

> On Sat, 2010-08-28 at 03:24 -0500, Yao G. wrote:
> > I did a quick dive on Johnathan code, and I think it's
> > superb. I think it deserves a review, because it could be a better
> > candidate to inclusion that mine. I'll continue porting the Boost date
> > time library, because as I'm using it on my own code, but obviously the
> > better one should be picked.
>
> I haven't had time to really test Jonathan's code, but I've looked at it and I agree it looks very good.  Which one is the better of Yao's and Jonathan's, I cannot say.  They both have their strengths, and neither is complete yet.
>
> Perhaps we should ask for a vote on the NG?
>

Is it too late for them to merge their code?


Philippe
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20100828/28737cd2/attachment.html>

On Saturday 28 August 2010 07:27:36 Philippe Sigaud wrote:
> On Sat, Aug 28, 2010 at 16:21, Lars Tandle Kyllingstad <lars at kyllingen.net>wrote:
> > On Sat, 2010-08-28 at 03:24 -0500, Yao G. wrote:
> > > I did a quick dive on Johnathan code, and I think it's
> > > superb. I think it deserves a review, because it could be a better
> > > candidate to inclusion that mine. I'll continue porting the Boost date
> > > time library, because as I'm using it on my own code, but obviously the
> > > better one should be picked.
> > 
> > I haven't had time to really test Jonathan's code, but I've looked at it and I agree it looks very good.  Which one is the better of Yao's and Jonathan's, I cannot say.  They both have their strengths, and neither is complete yet.
> > 
> > Perhaps we should ask for a vote on the NG?

That would probably be better once they're more complete.

> Is it too late for them to merge their code?

I haven't yet had time to look at Yao's code in detail, but I believe that he's more or less directly porting Boost's internal implementation of date and its associated classes. I specifically deviated from that, because I didn't think that it was the most efficient way to do it - either in terms of efficiency of space or efficiency of execution. Some operations are likely faster in Boost's implementation than mine (most notably getting the current day of the gregorian calendar, since it keeps the number of days as its internal state), but it has to convert from its internal representation of time for almost every function you call. Mine has the year, month, and day separately, which not only takes less memory, but it means that you can get those fields (likely oen of the most common operations) without having to do any calculations, and a number of the other calculations are probably more efficient because it has them separately. So, I think that the most common operations are going to be faster, and it takes the memory. I'd have to benchmark it though to see exactly what is and isn't faster. Also, my implementation makes it easy to add special dates like infinity without any of the weird stuff that Yao is being forced to do due to how Boost does it.

In any case, I don't think that there's a whole lot to merge. They specifically took different paths in how to implement their internals, and the APIs should be almost identical (since their APIs are both based on Boost's), with the exception of additional functionality that each of us has added. You could probably try and merge my TimeOfDay and DateTime (which combines Date and TimeOfDay) code with Yao's Date code, but I don't think that there's really any gain there unless it were determined that Yao's code is better.

I think that it would be of greater value for us both to continue what we're doing, and then compare the results. If one is clearly better, then we can go with that one. If one is better but is lacking in comparison to the other, then we can port over some of the missing functionality.

In any case, for the moment, I'm plowing along with what I've got, and while the APIs may be similar for the Date stuff, I expect that the time interval stuff will differ quite a bit more, since I'm trying to thoroughly D-ify it with ranges and delegates. I also combined the duration stuff for all types of time points rather than having separate ones for each type like Boost does, so a good chunk of it will likely be more generic than what Boost has. So, the overall result could be quite a bit different in some regards.

So, some level of merging of code may make sense later, but for now, I think that it would be best to just keep them separate and compare and contrast them, looking at the pros and cons of each, when they're more or less done.

- Jonathan M Davis