[Monotone-devel] Re: nvm.dates review

[Markus Wanner]

Hello Zack,

thank you very much for your in-depth review.

Zack Weinberg wrote:
> Under what circumstances would one want a date_t with an invalid value?

The --date option is either given or not. We are parsing dates from
strings pretty often, this might fail or not. And in
nvm.cvsimport-branch-reconstruction, I have a pre-calculated average
date or not, in which case I need to calculate such an average.

All in all, I think you'd more often have to write the following than not:

struct {
   bool date_given_and_valid;
   date_t the_date_itself;
}

instead of just using date_t as we do now. Also note, that
date_t::valid() has been there before.

> 
> +  // initialize from a unix timestamp
> +  date_t(u64 d);
> 
> I am not sure I like having this be a constructor instead of a factory
> function.  I don't feel strongly about it, but it was more
> self-documenting the other way.  Also, you still have the ::now() and
> ::from_string() factories, so now there's both factories and public
> constructors, which is confusing.

I felt the factory function to be overly verbose. Guess that's a matter
of taste. I didn't change that for now.

>  Also, you still have the ::now() and
> ::from_string() factories, so now there's both factories and public
> constructors, which is confusing.

There's the following constructor as well:

>   // initialize from multiple values
>   date_t(int sec, int min, int hour, int day, int month, int year);

Shall that be converted back to a factory function as well?

> Also, throughout, please make all your comments be complete sentences.

Okay, I'll try.

> +  date_t(int sec, int min, int hour, int day, int month, int year);
> 
> Why doesn't this take milliseconds as well?

Because we don't really need to initialize with milliseconds anywhere.
Nor does the string output function print them. It was trivial enough to
add it, though, so I just did.

> No "struct", please (same for all the others)

Okay, corrected.

> +  void gmtime(struct tm & tm) const;
> +  void mktime(struct tm const & tm);
> 
> Two problems here: first, you must not use struct tm for internal
> interchange.

Yes, I've had that in mind, but then forgot about it, sorry. That's now
corrected. Including using saner month (1 - 12) and year (1900 is 1900
and not 0) representation.

> Second, as a matter of clarity, please do not give any member function
> (of anything) the same unqualified name as a C library function!  I
> was quite confused the first time I read the code.

Hehe, sorry.. well, it took me quite a while to figure out that the
former method "from_unix_epoch" did about the same as "mktime", but
didn't mention it anywhere.

To keep it clear that these methods are about equivalent to the system
functions, I've now prefixed them with "our_". Feel free to rename to
whatever else you find more appropriate, but please keep it clear that
these provide similar functionality.

> +date_t::date_t(int sec, int min, int hour, int day, int month, int year)
> +{
> +  // general validity checks
> +  I((year >= 1970) && (year <= 9999));
> 
> Here and elsewhere - since you took out the buffer that would be too
> small in CE 10000, why are you still restricting the year to <= 9999?

Because that's been the limit before in date_t::now() and I thought 9999
is far enough in the future for our use.

> The year limit you should enforce is 2147483647, since all system
> structures for broken-down time that I am aware of (i.e. struct tm +
> the several similar things Windows has) use a 32-bit *signed* year
> field.  The corresponding seconds-since-1970 count was in the old
> version of the file, although of course it'll need multiplying by
> 1000.

Given that we don't even want to check dates between revisions, I don't
see much point in such general purpose date handling routines. I'll see
if I get around extending the limit anyway, but I'd like doing things
more relevant for monotone again.

> sec <= 60.  Leap seconds do happen.

..but we don't support them. Nor did the former (nor current)
"from_string" parsing function.

If we want to add support for leap seconds, that should clearly be done
separately. As long as we don't, I think it's better making sure
our_mktime and our_gmtime are complementary.

> Since your endpoint format is a millisecond count, why do the
> expensive conversion to broken-down time and back?  All you need to do
> is calculate and memoize the epoch offset used by this system, and
> then add that value to what you get back from time() and multiply by
> 1000.

We are currently using date_t::now() at exactly one place:
put_standard_certs_from_options(). So this clearly is a micro
optimization. Storing the known_epoch_offset doesn't help much, because
it's barely ever required twice per mtn execution.

However, as you've already written the code, I've added it. Converting
our_mktime to support years below 1970 uncovered a bug therein, so it
was even worth the effort ;-)

> (Since the internal representation is in milliseconds, it would be
> nice to use gettimeofday() where available, but that's not urgent.)
> 
> -unsigned int const MIN = 60;
> -unsigned int const HOUR = MIN * 60;
> -unsigned int const DAY = HOUR * 24;
> -unsigned int const YEAR = DAY * 365;
> -unsigned int const LEAP = DAY * 366;
> +u64 const SEC = u64_C(1000);
> +u64 const MIN = 60 * SEC;
> +u64 const HOUR = 60 * MIN;
> +u64 const DAY = 24 * HOUR;
> +u64 const YEAR = 365 * DAY;
> +u64 const LEAP = 366 * DAY;
> 
> Why was this necessary?

The number of milliseconds in a year exceeds 2^32. Other operations must
be done in 64bit as well, so I concluded it's safer to use 64bit
operations and types everywhere.

> Another advantage of not using struct tm is, you could report the
> milliseconds too.

Uh.. we certainly don't want that for date certs, do we? Any other use
cases?

> This function should maybe have the word "millisecs" somewhere in its
> name.

Agreed, renamed to millisecs_since_unix_epoch().

> It might be more efficient to divide out the milliseconds at the very
> beginning.  (And come to think of it, you could do seconds, minutes,
> and hours that way too, and deal in day counts for the rest of the
> code, which might be more comprehensible than how I wrote it
> originally.)

I simply required the ability to do some +/- calculations on these
dates. I do clearly not have the intention to write a general purpose
and high performance date_time handling library for C++.

> It's fractional seconds (you could have an arbitrary number); why
> ignore them, when you support millisecond precision?

Because we don't need them. It's simple enough to add as soon as we do.

None the less, I've now added milliseconds support for our_gmtime and
our_mktime. If you get the time to improve the parser as well, please go
ahead. However, as you say yourself, we should maybe better look into
gnulib's parser.

> I expect this will be going away again after you take out the
> four-digit year check.

Not sure I get around increasing that limit anytime soon, but I promise
to take care of it before year 2050, which should still be soon enough. ;-)


Again, thank you for your review.

Please bear in mind what the purpose for this change was/is: adding the
ability to get timestamp differences, adding and subtracting differences
to them. IMO this has already taken way too long and we should better
spend our dev-time on more relevant things.

Regards

Markus Wanner