Hi,
Enclosed is a patch that documents all of the dc_datetime functions.
The CAVEATS are important because calling gmtime et al from a library might not be expected. (They touch the zoneinfo files, which may not be available on embedded or sandboxed systems.)
I also specifically note the differences to "struct tm" in terms of the year and month field.
Best,
Kristaps
Enclosed is a patch that documents all of the dc_datetime functions.
The CAVEATS are important because calling gmtime et al from a library might not be expected. (They touch the zoneinfo files, which may not be available on embedded or sandboxed systems.)
I also specifically note the differences to "struct tm" in terms of the year and month field.
Ignore previous.
Resent with dc_datetime_gmtime and dc_datetime_localtime updated to reflect that "result" is filled in and what happens if "result" is NULL.
Best,
Kristaps
On 2017-01-12 10:50, Kristaps Dzonsons wrote:
Enclosed is a patch that documents all of the dc_datetime functions.
Looks good, except for one thing:
The CAVEATS are important because calling gmtime et al from a library might not be expected. (They touch the zoneinfo files, which may not be available on embedded or sandboxed systems.)
Why would that be unexpected? They are standard C library functions. I see no reason not to use them (and there are basically no easy alternatives). If their implementation is broken on some system, then that's a libc problem. So if you ask me, this caveats section is not necessary and only adds confusion.
PS: I do use the thread-safe variants (e.g. localtime_r and gmtime_r) when available.
Jef
Looks good, except for one thing:
The CAVEATS are important because calling gmtime et al from a library might not be expected. (They touch the zoneinfo files, which may not be available on embedded or sandboxed systems.)
Why would that be unexpected? They are standard C library functions. I see no reason not to use them (and there are basically no easy alternatives). If their implementation is broken on some system, then that's a libc problem. So if you ask me, this caveats section is not necessary and only adds confusion.
Jef,
It's not that libc is broken w/r/t these functions, but it should be noted that they're being used to specify how localtime is being computed.
(Why? First, because according to POSIX, localtime_r might invoke tzset() beforehand, or it might not. Moreover, in chroots or in capabilities environments, localtime might behave like gmtime due to file-system restrictions.)
Perhaps this should be in an IMPLEMENTATION NOTES section instead of the CAVEATS section, however, because it's not a bad thing --- just clarification.
(For safety, it should also be noted in dc_parser_get_datetime that the dc_datetime_localtime function might be invoked.)
I forgot to edit Makefile.am in my patch. If you'd like, I can rephrase the CAVEAT as an IMPLEMENTATION NOTE and amend the Makefile.am in another patch?
Best,
Kristaps
On 2017-01-12 15:34, Kristaps Dzonsons wrote:
Looks good, except for one thing:
The CAVEATS are important because calling gmtime et al from a library might not be expected. (They touch the zoneinfo files, which may not be available on embedded or sandboxed systems.)
Why would that be unexpected? They are standard C library functions. I see no reason not to use them (and there are basically no easy alternatives). If their implementation is broken on some system, then that's a libc problem. So if you ask me, this caveats section is not necessary and only adds confusion.
It's not that libc is broken w/r/t these functions, but it should be noted that they're being used to specify how localtime is being computed.
(Why? First, because according to POSIX, localtime_r might invoke tzset() beforehand, or it might not. Moreover, in chroots or in capabilities environments, localtime might behave like gmtime due to file-system restrictions.)
Perhaps this should be in an IMPLEMENTATION NOTES section instead of the CAVEATS section, however, because it's not a bad thing --- just clarification.
Yes, I think that's better. With the caveats section and the current description ("might interact with the file-system in unexpected ways"), it sounds as if there is a problem. And that's not really true.
I also suggest to rephrase the paragraph in a more informative way. For example something like this:
"The implementation of the dc_datetime_gmtime function may be based on the standard C library time functions, like gmtime(3) or gmtime_r(3)."
BTW, the main reason why those functions are exposed in the first place is to give an application access to exactly the same functions as used by libdivecomputer internally. For simple C/C++ applications, which are using the C library functions too, that doesn't matter. But applications using some kind of framework (Gtk, Qt, .NET, python, etc) usually have their own date/time functions, which may use a completely different implementation (especially on Windows).
(For safety, it should also be noted in dc_parser_get_datetime that the dc_datetime_localtime function might be invoked.)
Isn't that obvious? A function to parse date/time will probably use the library's date/time functions :-)
I forgot to edit Makefile.am in my patch. If you'd like, I can rephrase the CAVEAT as an IMPLEMENTATION NOTE and amend the Makefile.am in another patch?
I did already add the missing Makefile.am bits in my tree. So just the manpages changes.
Jef
Jef,
Yes, I think that's better. With the caveats section and the current description ("might interact with the file-system in unexpected ways"), it sounds as if there is a problem. And that's not really true.
I agree: enter version 3! (Minus the Makefile.am bits.) This just has the notice as a separate paragraph.
An unrelated note: I found that sometimes you use dc_datetime_gmtime and sometimes dc_datetime_localtime for the dc_parser_get_datetime value. So is it correct to say that the value always returns the broken-down localtime? (Specifically: the shearwater predator, suunto eon steel, and uwatec smart computers invoke gmtime. The uwatec specifically notes the rationale behind this, however.)
Isn't that obvious? A function to parse date/time will probably use the library's date/time functions :-)
True, but it doesn't help to be specific! I just included a note that it calls to the dc_datetime_gmtime or dc_datetime_localtime functions.
Best,
Kristaps
On 12-01-17 18:17, Kristaps Dzonsons wrote:
Yes, I think that's better. With the caveats section and the current description ("might interact with the file-system in unexpected ways"), it sounds as if there is a problem. And that's not really true.
I agree: enter version 3! (Minus the Makefile.am bits.) This just has the notice as a separate paragraph.
I've applied your patches.
An unrelated note: I found that sometimes you use dc_datetime_gmtime and sometimes dc_datetime_localtime for the dc_parser_get_datetime value. So is it correct to say that the value always returns the broken-down localtime? (Specifically: the shearwater predator, suunto eon steel, and uwatec smart computers invoke gmtime. The uwatec specifically notes the rationale behind this, however.)
Yes, it's always local time. That's what users expect to get (and what most dive computers record). Whether we need to use localtime or gmtime depends on how the dive computer records its timestamp.
Jef
-
Jef Driesen -
Kristaps Dzonsons