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

From: Gennadiy Rozental (rogeeff_at_[hidden])
Date: 2002-04-23 15:29:40

Hi,

I must admit I am not an expert in date/time staff. But here some of my
thoughts from preliminary review.

1. Generic base
I did not agree with design of generic base of the library. Separation of
date and time does not look reasonable. 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 ):

Generic class time_system_level

date_time_system_policies defines
n - number of levels
level_unit_types - list of unit types for each level
level_types        - list of types for each level (constrained by next level
size)
conversion functions that converts units of any type to level_type of any
other level in form of templated convert function
...

template<int level_id,typename date_time_system_policies>
class date_time_system_level {
public:
    typedef
date_time_system_policies::level_unit_types::get<level_id>::unit_type
unit_type;

    date_time_system_level( unit_type );

ate_time_system_level( 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 );
    date_time_system_level( normalized_value<n,level_id> );

    template<int target_level>
    date_time_system_policies::level_unit_types::get<level>::level_type,
    convert_to();

    normalized_value<n,level_id>
    normalize();

    comparison operators

    is_valid() // ?? why do we need that? Could not we make sure that value
is valid always and throw an exceptions on invalid operations?
    // is infinity should be implemented by means of defining spec values
for each level that are positive and negative infinity. Then use comparison
operations

    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.

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.

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

4. date_duration, time_duration
a. class date_duration does not support: dd < 3
b. why do we need is_negative
c. invert_sign -> operator-
d. it is one class really. why different interfaces?

5. Syntax comments
  a. no tabs,
  b. fix naming conventions,
  c. why semicolon after brackets like this: };
  d. why short name time_itr instead of time_iterator?
  e. Code have lot of comments like this: //  std::cout
  f. examples should include using <> instead of ""

6. Does not belong to the library
a. constrained_value :
b. period template seems to look like general range class.
c. struct ulong_date_traits is defined but is not used
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

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
c. why date_resolutions is defined in date_iterator (and never used) while
time_resolutions in separate header

8. Factors
month_functor: logic very unclear should be documented in comments

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.

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

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.

Gennadiy.