Re:[boost] Date/Time formal review

From: Gary Powell (powellg_at_[hidden])
Date: 2002-04-23 12:15:00

First, I've been waiting for this library ever since I heard Jeff give his
talk in Tampa FL.

I vote for acceptence, with the following fixes to be applied as required.

Code comments:
gregorian/formatters.hpp
to_iso_string, to_iso_extended_string, and to_simple_string, should
probably take a const & date. to_iso_string and to_simple_string should
take a const date_period

I'm also not sure, but it is probably worth using the template
"basic_string", instead of std::string. That way you can have w_char if
its supported. (That is to say, write these inline functions as function
templates.) 

gdtl/adjust_functors.hpp
get_offset should take a const &date_type.
(or boost::call_traits<date_type>::const_reference d)

Should the "week_functor" instead of multiplying by "7", get this value
from the date_type?  I know it sounds weird but "7" is arbritary. And one
could imagine a Martian calender with more or less days in a week than 7.

Also as a general coding style I'd add "explicit" to all the default
constructors and those taking only one non default argument. This seems to
catch a few coding errors of unintentional conversion.

In date_clock_device.hpp
the references to the namespace "std", need leading "::",as in

"::std::local_time(t);"

Otherwise its looking in boost::gdtl::std::local_time which shouldn't work
on non broken compilers.
(Same in int_adapter.hpp, time_clock.hpp)

date_formating.hpp
class month_formatter
I think the switch statement could be written as a compile time switch?

If format_type returned different types for the call ::month_format(). As
it looks like once the class is instantiated, its always the same result.

(same for ymd_to_string, in ymd_formatter)

Point of coding style:
  if (cond)
  {
    return;
  }
  else
  {
    return;
  }

should be written

  if (cond)
  {
    return;
  }

  return;

Why? Some compilers don't eliminate the extra jump instruction, also it
removes unnecessary indentation/nesting.

  if (cond)
  {
     if (cond2)
       return;
     else
       return;
  }
  else
  {
    if (cond3)
      return
    else
     return
  }
 

becomes
   if (cond)
   {
      if (cond2)
        return;

      return;
   }

   if (cond3)
      return;

   return;

(Example file period.hpp)

time_iterator.hpp
Shouldn't time_itr be declared with iterator_traits, of forward_iterator?

I guess the only thing I want is the ability to mix and match dates, as in
given a date "now", add 240 seconds, or - 1 week, or + 1 month and ask
"when". I missed this in the
documentation, but is the kind of application I have right now for
date/time.

  So while I like this library, I think there is more to be done. (Which
may be just adding some documentation to show some more examples.)

  Yours,
 Gary

powellg at a m a z o n d o t c o m