I've now pushed some revs to nvm.dates in which I made many of the changes you said you would rather not do. I feel I ought to explain why I'm insisting on those changes.
I have some further changes in mind for date handling, but I'm not going to get to them for a couple days. If you would like to merge nvm.dates and nvm.dates.statistics into mainline now, please go ahead. On Mon, Oct 27, 2008 at 8:42 AM, Markus Wanner <mar...@bluegap.ch> wrote: > Zack Weinberg wrote: >> + // 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. I made the ::from_string() factory into a constructor but left ::now() and the other constructors alone. That's enough consistency for me. >> + void gmtime(struct tm & tm) const; >> + void mktime(struct tm const & tm); > > 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. I changed "our_mktime" to "our_timegm". The system function mktime() operates in the local timezone so it is clearer not to use that name at all for a function that operates in GMT. Some (unfortunately not all) systems have timegm() that operates in GMT. >> 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. Practically speaking, 9999 is far enough in the future, I imagine by then we'll all be using something totally different. But making the code work as far out as you possibly can is good anyway, because it forces you to make the algorithm more robust. I made our_gmtime work as far out as I could (not to 2147483647 -- it turns out that we overflow a signed 64-bit millisecond count before then) and in the process was forced to come up with a technique for calculating the year that is just plain better, even in the date range we'll actually be using. >> 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? I haven't done this part yet, but we do actually want milliseconds in date certs, or at least we want to accept and preserve them when they show up. I don't remember the exact details, but conversion from some foreign formats produces date certs with milliseconds in them, which is why the from-string constructor currently accepts and ignores digits after the decimal point. > 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. I want to mention four more things that we should try to get out of our date handling. With your changes and mine on top of them, we're very close, but some more work is still needed. I'm planning to do all of this, but if you feel inspired to do 'em first, please go ahead. First, when we print dates, we should print 'em in local time, not GMT, and we should offer the user the option to choose their preferred date format. This involves adding another as_* function that takes a strftime() format string (which higher layers will get from a Lua variable) and runs the internal representation through the system localtime() function. This is also another use for get_epoch_offset and is what I had in mind when I requested it, btw. Second, similarly, the --date option should operate in local time by default. The headache with this is that while mktime() is standard, filling in a 'struct tm' correctly is harder than one might think (as I mentioned before); the nonstandard 'tm_gmtoff' and 'tm_zone' fields may affect the result. (The Linux manpage doesn't say anything about them; glibc does have those fields but I *think* its mktime ignores them; the FreeBSD manpage mentions the fields but doesn't say what mktime does with them; I would want to survey other systems before actually doing this part.) Third, I do think we should request millisecond-accurate timestamps in now(), pass them through date certs, and print them when possible. This will be tricky in the face of the first one because strftime() and struct tm don't have millisecond fields -- I really don't want to implement my own strftime(). Maybe we could get away with a subsequent call to sprintf() or FL() if we see some kind of marker in the string. And then, of course, there is the question of more general date parsing. I'm actually tempted to do that in Lua, maybe translating Mark Pilgrim's Universal Feed Parser's date parser into that language, and leave the from-string constructor as a fast routine that only knows the date cert format. zw _______________________________________________ Monotone-devel mailing list Monotone-devel@nongnu.org http://lists.nongnu.org/mailman/listinfo/monotone-devel