Re: [boost] [gsoc 2013] draft proposal for chrono::date

Subject: Re: [boost] [gsoc 2013] draft proposal for chrono::date
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2013-04-27 08:47:08

Le 27/04/13 10:55, Anurag Kalia a écrit :
>> Where is it?
> I am very sorry that I didn't upload the proposal itself! It just feels foolish. I have attached the original proposal here.
>   
>
Hi Kalia,

I will start with some concrete points on your proposal and finish with 
some points you have not covered.

* Do we need  a typedef for days_point? The usual way in chrono is to 
define clock and durations typedefs but not time_point typedefs.
typedef std::chrono::time_point<std::chrono::system_clock, days> day_point;

*  Why do you want date to inherit from this day_point? Do you want to 
make implicit conversions from date to minutes or seconds? I would 
instead define an explicit getter function otherwise you would inherit 
form all the time_point<> operations which is not what we want.

* I don't know if your day_clock is a digression or you want to define 
it. In you define it it seems natural that your day_point would be instead
typedef days_clock::time_point day_point;

But then you would not have the implicit conversion to a 
system_clock::time_point.

* What could be the use of the typedef month31, month30, month28, 
month29, year_normal and year_leap and the associated types jan, 
feb_normal, feb_leap, ...?

* About the change_day, change_month and change_year.
First the name change_ doesn't mean add.
What is wrong with the days/month and year arithmetic that allows to 
just write

d + days(1) + month(1) + years(1)

instead of

d.change_day(1);
d.change_month(1);
d.change_year(1);

* chrono::duration for months/years
I would use instead average_months and average_years

* H.H proposal as well as my adaptation use a internal duration for 
months and years with closed arithmetic.
If you use your

typedef duration<double, std::ratio<2629746L>> months;

typedef duration<double, std::ratio<31556952L>> years;

then following

date next_month = today + months(1);

would not represent the next month but an approximation of the next 
month, that is today + the average number of hours of a month :(

* I/O
I don't like to associate the format with an instance of date. This 
spend memory and a user would expect the same format independently of 
how this instance has been constructed.
Would your provide input of a date from an input stream on your library?

* There is a typo in
date d1 = date(2) / month(4) / year(2011);

* Please don't use CamelCase for the Month and WeakDays

* I disagree with your point of view respect the use of _1st, _2nd, ... 
forbidding its usage for nth day of the month or the nth weekday of the 
month doesn't prevent you building invalid dates.
Note that you are able to build an invalid date even if you don't use 
these nth symbols, e.g.

day(31)/april/2013

If you want symmetry you could use use nth(1) and nth(-1) .

* Could you add to your proposal the description of the different 
representations you want to take in account?

Now I will continue with the things that are not in the proposal:

* one important thing is how and when dates are validated? Can the user 
construct invalid dates?
* If you propose to have several date classes with different 
representation you need to clarify how the conversions of the different 
representations is done?
* Would your proposal improve the performances?
* What your interface adds to the one proposed by H.Hinnant? Why this is 
an improvement for your?

I understand that you want to design your own library, I did the same, 
but getting a coherent design would take you some time.
I suggest you to start with the H. Hinnant proposal as a reference and 
state what you want to added, changed or removes. State clearly why do 
you want these changes, what would be improved/solved.

Don't forget to inspect the threads/discussion related to the H.Hinnant 
proposal, and the paper N3344 Toward a Standard C++ 'Date' Class 
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3344.pdf

HTH,
Vicente