RE: [boost] Re: reminder about Date/Time formal review

From: Jeff Garland (jeff_at_[hidden])
Date: 2002-04-23 17:43:48

> 1. Generic base
> I did not agree with design of generic base of the library. Separation of
> date and time does not look reasonable. 

Ok, but the basic premise is that a 'date only' library shouldn't depend on 
'time specific' things.  There are many programming problems that exist
strictly at the 'date level'.

>For one it's replication of code sometime. But mainly it's not flexible. What 
>if I need 3 level date/time system that have day, hour and seconds? Or 2 level 
>time system that have only weeks and days. Generic solution may look 
>something like this (from top  of my head, only sketch ):

This is done by replacing the time system and supporting types.

> Generic class time_system_level
> ...snip implementation suggestion...

I think this would be an interesting approach to explore.  What you're
suggesting (assuming I understand it and we could get it to compile)
could unify the idea of the concept of time point.

>     is_valid() // ?? why do we need that? 
>Could not we make sure that value
> is valid always and throw an exceptions on invalid operations?

Because some date-time systems will allow of 'not-a-date-time' 
values.  They need to be able to query for this state before
they get the exception.

>     operator-,+ will use some other class that encapsulate duration instead
> of two similar classes date_duration, time_duration
> 
>     read-only property: unit_type    m_value;
> }
> 
> normalized_value<n,level_id> :=
> uple( date_time_system_policies::level_unit_types::get<n>::level_type,
> 
> date_time_system_policies::level_unit_types::get<n-1>::level_type,
>                                                            ...
> 
> date_time_system_policies::level_unit_types::get<level_id>::level_type )
> 
> 
> Then we need one generic iterator through level. Some generic io support.
> There may be another level of generality that provide current separation.
> Also we need some generic support for conversions.

I'm not following here....
 
> 2. I/O
> I/O functionality supplied is very inflexible. It should be completely
> managed by some user supplied (with some defaults) format string.
> date_time_system_policies could assign specific letter to each level and
> that user could define arbitrary formats. Also to/from_string interface is
> not acceptable. It should be iostream based one.

I'm sure no matter what is done someone won't be happy with the I/O.  I specifically
didn't want to write a 'format string' capability.  This is a complex job that
would consume a great deal of time.  I would be happy to incorporate this if
someone else wants to take it on. As for inflexible, you are always free
to extract the elements of a date-time and write your own formatting routines.

> 3. Date generators
> I see it as more static facility. So I would expect day, month, year to be
> template parameters. Also we could provide convenient typedefs like this:
> date_generator::April::first
> date_generator::first::Sunday::may, date_generator::second::Monday::august,
> ...,
> date_generator::last::Thursday::september

Could be, but this won't work for programs that read the specification
as data and need to create dates.  I don't know if there is a strong
argument for having both...

> 4. date_duration, time_duration
> a. class date_duration does not support: dd < 3

Right.

> b. why do we need is_negative
> c. invert_sign -> operator-

Yes, that's better.

> 5. Syntax comments
>   a. no tabs,

I don't think there are any, but I'll check...

>   b. fix naming conventions,

Such as?

>   c. why semicolon after brackets like this: };

Hyperactive fingers :-)

>   d. why short name time_itr instead of time_iterator?

Lazyness -- will fix.

>   e. Code have lot of comments like this: //  std::cout

Debug cruft -- will fix.

>   f. examples should include using <> instead of ""

Disagree.  I subscribe to the <> for std library only camp....
 
> 6. Does not belong to the library
> a. constrained_value :

Agreed it can stand alone, but until someone else wants to use
it we will need a place for it.

> b. period template seems to look like general range class.

Yes it is.  Same comment as above.

> c. struct ulong_date_traits is defined but is not used

It's usage got removed and I forgot to remove it. Consider it gone.

> d. wrapping_int, wrapping_int2 does not belong to the library, wrapping_int2
> does not define subtract while wrapping_int does, in general interface is
> questionable

These are utility classes.  In a way, they are 'details' of the implementation.
That said, someone building a new time system might find them handy.
 
> 7. time_iterator and date_iterator
> a. time_iterator should use iterator_adaptor
> b. date_iterator should use iterator_adaptor and should not be using virtual
> functions, really should be one type

You are correct that these should use iterator_adaptors -- I haven't had a
chance to look at this yet.  The problem is that date_iterator is really 
solving a different problem.  It is allowing the user to plug-in a calculation
functor so that it can do things like figure adjust for the end of months
appropriately.  So maybe it needs a different name like logical_iterator.

> c. why date_resolutions is defined in date_iterator (and never used) while
> time_resolutions in separate header

hmmm, I'll look at that.
 
> 8. Factors
> month_functor: logic very unclear should be documented in comments

Ok.
 
> 9. Misc.
> c_local_time_adjustor, local_time_adjustor: does not seems to belong to
> generic framework. probably should be moved in conversions subsystem.
> day_clock, second_clock : does not seems to belong to generic framework.
> probably should be moved in conversions posix subsystem.

No. These could be used for other date-time systems.  
 
> 10. Compilation on MSVC issued following warnings:
> \gdtl_057_boost\boost\gdtl\gregorian\gregorian_calendar.ipp(27) : warning
> C4244: 'return' : conversion from 'unsigned long' to 'short', possible loss
> of data
> \gdtl_057_boost\boost\gdtl\gregorian\gregorian_calendar.ipp(58) : warning
> C4244: 'initializing' : conversion from 'unsigned long' to 'unsigned short',
> possible loss of data
> \gdtl_057_boost\boost\gdtl\gregorian\gregorian_calendar.ipp(59) : warning
> C4244: 'initializing' : conversion from 'unsigned long' to 'unsigned short',
> possible loss of data
> \gdtl_057_boost\boost\gdtl\gregorian\gregorian_calendar.ipp(63) : warning
> C4244: 'argument' : conversion from 'unsigned long' to 'unsigned short',
> possible loss of data

These are acceptable.  I suppose we should try to suppress them.
 
> FWIW I vote NO to accept GDTL as *generic* date time library. It could be
> useful (except io staff ) right away for some application but generic
> library should demonstrate more generic design. I will be happy to change my
> mind if I completely misunderstood the issue.

Well, I believe you should.  I'll be the first to acknowledge that the design
can be improved and likely be made more generic as well.  However, 
I don't believe that the main point of GDTL is to maximize 'genericity'.
My interest is in providing valuable tools and algorithms for the 
construction of date-time systems and I see the current library as
a base.  Even in it's current form I believe it offers things not
commonly available in other date-time libraries.  Regardless of 
genericity I contend that the library is still better factored 
into reusable parts better than other date-time libraries.  

And acceptance doesn't we can't explore and replace things if we 
find better ways.  In fact I believe acceptance will in fact
improve the likelihood of this because it will raise issues 
important to real users. Certainly the review comments will improve 
the library design and implementation greatly.  But I believe if 
the library is rejected now, it damages the prospect of evolution 
in the future because there will be less usage and hence less
feedback.  In addition, there are some users that need solutions 
now that will have all of their needs fulfilled by the current 
date-time systems.  These folks will have their life simplified
if the library is integrated into boost instead of 'in-limbo'.

Jeff