Hi Bruno,
Bruno Haible <[email protected]> writes:
> Collin Funk wrote:
>> The first patch is a prerequisite to fixing a GNU coreutils bug. It adds
>> a posixtime_z function which operates on a timezone_t instead of the TZ
>> environment variable.
>
> Why do you add the timezone_t as first argument?
>
> The common rules / guidelines for argument ordering in C APIs are:
> - If there are only input parameters, the arguments are sorted
> so that the ones with most variation come first and the ones
> which are the same among different invocations come last.
> - Input parameters that belong together (semantically) come close
> together.
> - If there is an output parameter, this argument comes first,
> before all input parameters.
>
> By the third of these guidelines, I would define the function as
>
> bool posixtime_z (time_t *p, timezone_t tz, const char *s,
> unsigned int syntax_bits);
>
> Note that this is not the same situation as for mktime_z.
> mktime_z has a parameter that is input/output, not just output.
> But for mktime_z too I would have put the timezone_t argument in
> the second place; I don't know why Paul chose to put it first in 2015.
I actually agree with your preferences on the ordering. The reason I
wrote it the way I did is because the ctime_rz, localetime_rz, and
mtime_z functions take the timezone_t parameter first. Those ones all
return the result last, instead of first, though, so even my function
isn't really in agreement with them. I'll make that change, since having
the returned value somewhere in the middle does feel a bit too strange.
Regarding the choices that Paul made in 2015, these functions are NetBSD
inventions [1]. Also see the note on tzdb's home page, under the section
"Extensions to POSIX in the 'tz' code" [2]:
Functions tzalloc, tzfree, localtime_rz, and mktime_z for
more-efficient thread-safe applications that need to use multiple
timezones. The tzalloc and tzfree functions allocate and free
objects of type timezone_t, and localtime_rz and mktime_z are like
localtime_r and mktime with an extra timezone_t argument. The
functions were inspired by NetBSD.
So, I assume Paul was just being nice by not giving everyone conflicting
prototypes to deal with.
> That is problematic. It removes the only unit test of the posixtime
> function, and thus also makes it a mystery how to call the posixtime
> function correctly.
>
> Instead of removing the posixtime's tests, can you instead arrange to
> have two unit test functions: one that demonstrates and tests posixtime_z,
> and one that demonstrates and tests posixtime?
True. It should also probably also do some checks using a non-UTC time
zone. I see in tests/test-localtime_r-mt.c we define FRENCH_TZ, so I'll
see if that can be used without too many portability issues. I'll make
those changes tomorrow. Thanks for the review.
Collin
[1] https://man.netbsd.org/mktime.3
[2] https://www.iana.org/time-zones/theory
