diff -ruN S-vanilla/dates.hh S-dates/dates.hh --- S-vanilla/dates.hh 2008-05-13 20:07:26.693813111 -0700 +++ S-dates/dates.hh 2008-10-25 17:07:22.986962447 -0700 @@ -19,31 +19,62 @@
struct date_t { - // For the benefit of the --date option. - date_t() : d("") {} - bool valid() const { return d != ""; } + // initialize to an invalid date + date_t() : d(-1) {} Under what circumstances would one want a date_t with an invalid value? + // 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. Also, throughout, please make all your comments be complete sentences. + // initialize from multiple values "Initialize from broken-down time" would be clearer. That is the term used by the documentation of the C <time.h> functions that deal in this representation. + date_t(int sec, int min, int hour, int day, int month, int year); Why doesn't this take milliseconds as well? ... + // Date comparison operators + bool operator <(struct date_t const & other) const No "struct", please (same for all the others) + // Our own gmtime function which converts the internal Unix epoch + // into a struct tm. + 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. It is not as portable as one might think (yes, it's in C89, but BSD, SysV, and Windows all extended it in incompatible ways), and because of those extensions it is unsafe to fill one in by hand. You should use it only in the few cases where you have to use a <time.h> function that takes or returns it, and you should strive to avoid doing that wherever possible. Fixing this is a requirement for merging back to n.v.m. 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. +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? 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. + I((month >= 1) && (month <= 12)); + I((day >= 1) && (day <= 31)); + I((hour >= 0) && (hour < 24)); + I((min >= 0) && (min < 60)); + I((sec >= 0) && (sec < 60)); sec <= 60. Leap seconds do happen. +bool +date_t::valid() const +{ + // year 10000 limit + return d < u64_C(253402300800000); See above. + // This is the only place we rely on the system's time and date function + // which might operate on different epochs (i.e. 1980-01-01, as some + // Windows, old MacOS and VMS systems used). We immediately transform that + // to a struct tm representation, which is independent of the system's + // epoch. time_t t = time(0); struct tm b = *gmtime(&t); 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. static s64 get_epoch_offset() { static s64 epoch_offset; static bool know_epoch_offset = false; if (know_epoch_offset) return epoch_offset; std::time_t epoch = 0; std::tm t = *std::gmtime(&epoch); // you should use the internal function that does this directly, of course; // and take care it doesn't bomb out if the offset is negative // (system epoch earlier than unix epoch) epoch_offset = date_t::from_broken_down_time(t.tm_sec, t.tm_min, t.tm_hour, t.tm_mday, t.tm_mon + 1, t.tm_year + 1900) .as_unix_epoch(); know_epoch_offset = true; return epoch_offset; } date_t date_t::now() { std::time_t t = std::time(0); date_t d; d.d = u64(t)*1000 + get_epoch_offset(); return d; } Easy! (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? +string +date_t::as_iso_8601_extended() const +{ + struct tm tm; + gmtime(tm); + return (FL("%04u-%02u-%02uT%02u:%02u:%02u") + % (tm.tm_year + 1900) % (tm.tm_mon + 1) % tm.tm_mday + % tm.tm_hour % tm.tm_min % tm.tm_sec).str(); +} Another advantage of not using struct tm is, you could report the milliseconds too. +u64 +date_t::as_unix_epoch() const +{ + return d; +} This function should maybe have the word "millisecs" somewhere in its name. +void +date_t::gmtime(struct tm & tm) const { // these types hint to the compiler that narrowing divides are safe u64 yearbeg; - u32 year; + u16 year; u32 month; u32 day; - u32 secofday; + u32 msofday; u16 hour; - u16 secofhour; + u32 msofhour; u8 min; + u16 msofmin; u8 sec; + u16 msec; 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.) + // ignore milliseconds, if present, or go back to the end of the date + // string to parse the digits for seconds. It's fractional seconds (you could have an arbitrary number); why ignore them, when you support millisecond precision? + // make sure we are still before year 10'000 Please stick to English notation for numbers (i.e. 10,000) -- but this check will be changing anyway... // more than four digits in the year - OK("120070301T184113", "12007-03-01T18:41:13"); + NO("120070301T184113"); // equals 12007-03-01T18:41:13 I expect this will be going away again after you take out the four-digit year check. zw _______________________________________________ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel