On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> Hi,
>
> Couple things:
>
> - Use additional timespec variables to make our intent more obvious.
>
> Add "elapsed", "utc_offset", and "utc_start".
>
> "roff" is a confusing name, "utc_offset" is better.
>
> Yes, I know the clock is called CLOCK_REALTIME, but that's a
> historical accident. Ideally they would have called it CLOCK_UTC
> or CLOCK_UNIX. Sigh.
>
> - Before the loop, we only need to compute utc_offset in the -m flag
> case.
>
> - Before the loop, we only need to do two clock_gettime(2) calls in
> the -m flag case.
>
> - In the loop, we can use the new variables to help clarify what we're
> doing:
>
> + In the -i and -s flag cases we're using an elapsed time for the
> timestamp, so compute "elapsed".
>
> + In the default and -m cases we're using an absolute time for the
> timestamp, so (if necessary) compute "now".
>
> - We don't need to call clock_gettime(2) twice in the -i flag case.
> Just compute "elapsed" and then assign "now" to "start". Easy.
>
> - I think the pimp my ride joke is cute, but calling the function
> "print_timestamp()" is a bit more obvious. It also tells the
> reader that there's a side effect.
>
> - Because we always call localtime(3), we can move that call into
> print_timestamp() and just pass the timespec as argument.
>
> This makes it clearer where the nanosecond value is coming from,
> which in turn makes it clearer that the string "ms" will be exactly
> 6 bytes in length.
>
> I think "ms" stands for "microseconds", in which case a better name
> is "us" or "usecs", but that's outside the scope of this patch.
>
> --
>
> ok?
>
> Index: ts.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ts/ts.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 ts.c
> --- ts.c 4 Jul 2022 17:29:03 -0000 1.6
> +++ ts.c 4 Jul 2022 18:17:20 -0000
> @@ -32,7 +32,7 @@ static char *buf;
> static char *outbuf;
> static size_t bufsize;
>
> -static void fmtfmt(struct tm *, long);
> +static void print_timestamp(const struct timespec *);
> static void __dead usage(void);
>
> int
> @@ -40,8 +40,7 @@ main(int argc, char *argv[])
> {
> int iflag, mflag, sflag;
> int ch, prev;
> - struct timespec roff, start, now;
> - struct tm *tm;
> + struct timespec elapsed, now, start, utc_offset, utc_start;
> clockid_t clock = CLOCK_REALTIME;
>
> if (pledge("stdio", NULL) == -1)
> @@ -93,22 +92,28 @@ main(int argc, char *argv[])
> if (setenv("TZ", "UTC", 1) == -1)
> err(1, "setenv UTC");
>
> - clock_gettime(CLOCK_REALTIME, &roff);
> - clock_gettime(clock, &start);
> - timespecsub(&roff, &start, &roff);
> + if (clock != CLOCK_REALTIME) {
> + clock_gettime(clock, &start);
> + if (mflag) {
> + clock_gettime(CLOCK_REALTIME, &utc_start);
> + timespecsub(&utc_start, &start, &utc_offset);
> + }
> + } else
> + clock_gettime(CLOCK_REALTIME, &start);
>
> for (prev = '\n'; (ch = getchar()) != EOF; prev = ch) {
> if (prev == '\n') {
> clock_gettime(clock, &now);
> - if (iflag || sflag)
> - timespecsub(&now, &start, &now);
> - else if (mflag)
> - timespecadd(&now, &roff, &now);
> - if (iflag)
> - clock_gettime(clock, &start);
> - if ((tm = localtime(&now.tv_sec)) == NULL)
> - err(1, "localtime");
> - fmtfmt(tm, now.tv_nsec);
> + if (iflag || sflag) {
> + timespecsub(&now, &start, &elapsed);
> + if (iflag)
> + start = now;
> + print_timestamp(&elapsed);
> + } else {
> + if (mflag)
> + timespecadd(&now, &utc_offset, &now);
> + print_timestamp(&now);
> + }
> }
> if (putchar(ch) == EOF)
> break;
> @@ -132,11 +137,15 @@ usage(void)
> * so you can format while you format
> */
> static void
> -fmtfmt(struct tm *tm, long tv_nsec)
> +print_timestamp(const struct timespec *ts)
> {
> + struct tm *tm;
> char *f, ms[7];
>
> - snprintf(ms, sizeof(ms), "%06ld", tv_nsec / 1000);
> + if ((tm = localtime(&ts->tv_sec)) == NULL)
> + err(1, "localtime");
> +
> + snprintf(ms, sizeof(ms), "%06ld", ts->tv_nsec / 1000);
> strlcpy(buf, format, bufsize);
> f = buf;
>
>
I don't like the introduction of all these local variables that are just
hard to follow and need extra code pathes. Happy to rename roff to offset,
start_offset or something similar. Also moving the localtime call into
fmtfmt() is fine.
--
:wq Claudio
