Re: ts(1): make timespec-handling code more obvious
On Tue, Jul 05, 2022 at 07:04:49AM -0500, Scott Cheloha wrote:
> On Tue, Jul 05, 2022 at 11:53:26AM +0200, Claudio Jeker wrote:
> > On Tue, Jul 05, 2022 at 11:34:21AM +, Job Snijders wrote:
> > > On Tue, Jul 05, 2022 at 11:08:13AM +0200, Claudio Jeker wrote:
> > > > On Mon, Jul 04, 2022 at 05:10:05PM -0500, Scott Cheloha wrote:
> > > > > On Mon, Jul 04, 2022 at 11:15:24PM +0200, Claudio Jeker wrote:
> > > > > > On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > Couple things:
> > > > > > >
> > > > > > > [...]
> > > > > >
> > > > > > 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.
> > > > >
> > > > > You need an "elapsed" variable to avoid overwriting "now" in the
> > > > > -i flag case to avoid calling clock_gettime(2) twice.
> > > > >
> > > > > We can get rid of "utc_start" and just reuse "now" for the initial
> > > > > value of CLOCK_REALTIME.
> > > > >
> > > > > How is this?
> > > >
> > > > How about this instead?
> > >
> > > Looks like an improvement
> > >
> > > The suggestion to change 'ms' to 'us' might be a good one to roll into
> > > this changeset too.
> >
> > Ah right, we print us not ms.
> >
> > > nitpick: the changeset doesn't apply cleanly:
> >
> > Forgot to update that tree :)
> >
> > Updated diff below
>
> This is fine by me, you took most of what I wanted, and even
> the "ms" -> "us" name change :)
>
> One nit below, otherwise: ok cheloha@
>
> > 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.c4 Jul 2022 17:29:03 - 1.6
> > +++ ts.c5 Jul 2022 09:51:38 -
> > @@ -32,7 +32,7 @@ static char *buf;
> > static char*outbuf;
> > static size_t bufsize;
> >
> > -static void fmtfmt(struct tm *, long);
> > +static void fmtfmt(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 start, now, utc_offset, ts;
> > clockid_t clock = CLOCK_REALTIME;
> >
> > if (pledge("stdio", NULL) == -1)
> > @@ -93,22 +92,22 @@ main(int argc, char *argv[])
> > if (setenv("TZ", "UTC", 1) == -1)
> > err(1, "setenv UTC");
> >
> > - clock_gettime(CLOCK_REALTIME, );
> > clock_gettime(clock, );
> > - timespecsub(, , );
> > + clock_gettime(CLOCK_REALTIME, _offset);
> > + timespecsub(_offset, , _offset);
>
> You don't need to initialize utc_offset except in the -m flag case.
I did not do this because I think it is an micro-optimisation that is not
really worth it.
--
:wq Claudio
Re: ts(1): make timespec-handling code more obvious
On Tue, Jul 05, 2022 at 11:53:26AM +0200, Claudio Jeker wrote:
> On Tue, Jul 05, 2022 at 11:34:21AM +, Job Snijders wrote:
> > On Tue, Jul 05, 2022 at 11:08:13AM +0200, Claudio Jeker wrote:
> > > On Mon, Jul 04, 2022 at 05:10:05PM -0500, Scott Cheloha wrote:
> > > > On Mon, Jul 04, 2022 at 11:15:24PM +0200, Claudio Jeker wrote:
> > > > > On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> > > > > > Hi,
> > > > > >
> > > > > > Couple things:
> > > > > >
> > > > > > [...]
> > > > >
> > > > > 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.
> > > >
> > > > You need an "elapsed" variable to avoid overwriting "now" in the
> > > > -i flag case to avoid calling clock_gettime(2) twice.
> > > >
> > > > We can get rid of "utc_start" and just reuse "now" for the initial
> > > > value of CLOCK_REALTIME.
> > > >
> > > > How is this?
> > >
> > > How about this instead?
> >
> > Looks like an improvement
> >
> > The suggestion to change 'ms' to 'us' might be a good one to roll into
> > this changeset too.
>
> Ah right, we print us not ms.
>
> > nitpick: the changeset doesn't apply cleanly:
>
> Forgot to update that tree :)
>
> Updated diff below
This is fine by me, you took most of what I wanted, and even
the "ms" -> "us" name change :)
One nit below, otherwise: ok cheloha@
> 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 - 1.6
> +++ ts.c 5 Jul 2022 09:51:38 -
> @@ -32,7 +32,7 @@ static char *buf;
> static char *outbuf;
> static size_t bufsize;
>
> -static void fmtfmt(struct tm *, long);
> +static void fmtfmt(const struct timespec *);
> static void __deadusage(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 start, now, utc_offset, ts;
> clockid_t clock = CLOCK_REALTIME;
>
> if (pledge("stdio", NULL) == -1)
> @@ -93,22 +92,22 @@ main(int argc, char *argv[])
> if (setenv("TZ", "UTC", 1) == -1)
> err(1, "setenv UTC");
>
> - clock_gettime(CLOCK_REALTIME, );
> clock_gettime(clock, );
> - timespecsub(, , );
> + clock_gettime(CLOCK_REALTIME, _offset);
> + timespecsub(_offset, , _offset);
You don't need to initialize utc_offset except in the -m flag case.
Re: ts(1): make timespec-handling code more obvious
> > this changeset too. > > Ah right, we print us not ms. > > > nitpick: the changeset doesn't apply cleanly: > > Forgot to update that tree :) > > Updated diff below OK job@
Re: ts(1): make timespec-handling code more obvious
On Tue, Jul 05, 2022 at 11:34:21AM +, Job Snijders wrote:
> On Tue, Jul 05, 2022 at 11:08:13AM +0200, Claudio Jeker wrote:
> > On Mon, Jul 04, 2022 at 05:10:05PM -0500, Scott Cheloha wrote:
> > > On Mon, Jul 04, 2022 at 11:15:24PM +0200, Claudio Jeker wrote:
> > > > On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> > > > > Hi,
> > > > >
> > > > > Couple things:
> > > > >
> > > > > [...]
> > > >
> > > > 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.
> > >
> > > You need an "elapsed" variable to avoid overwriting "now" in the
> > > -i flag case to avoid calling clock_gettime(2) twice.
> > >
> > > We can get rid of "utc_start" and just reuse "now" for the initial
> > > value of CLOCK_REALTIME.
> > >
> > > How is this?
> >
> > How about this instead?
>
> Looks like an improvement
>
> The suggestion to change 'ms' to 'us' might be a good one to roll into
> this changeset too.
Ah right, we print us not ms.
> nitpick: the changeset doesn't apply cleanly:
Forgot to update that tree :)
Updated diff below
--
:wq Claudio
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.c4 Jul 2022 17:29:03 - 1.6
+++ ts.c5 Jul 2022 09:51:38 -
@@ -32,7 +32,7 @@ static char *buf;
static char*outbuf;
static size_t bufsize;
-static void fmtfmt(struct tm *, long);
+static void fmtfmt(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 start, now, utc_offset, ts;
clockid_t clock = CLOCK_REALTIME;
if (pledge("stdio", NULL) == -1)
@@ -93,22 +92,22 @@ main(int argc, char *argv[])
if (setenv("TZ", "UTC", 1) == -1)
err(1, "setenv UTC");
- clock_gettime(CLOCK_REALTIME, );
clock_gettime(clock, );
- timespecsub(, , );
+ clock_gettime(CLOCK_REALTIME, _offset);
+ timespecsub(_offset, , _offset);
for (prev = '\n'; (ch = getchar()) != EOF; prev = ch) {
if (prev == '\n') {
clock_gettime(clock, );
if (iflag || sflag)
- timespecsub(, , );
+ timespecsub(, , );
else if (mflag)
- timespecadd(, , );
+ timespecadd(, _offset, );
+ else
+ ts = now;
+ fmtfmt();
if (iflag)
- clock_gettime(clock, );
- if ((tm = localtime(_sec)) == NULL)
- err(1, "localtime");
- fmtfmt(tm, now.tv_nsec);
+ start = now;
}
if (putchar(ch) == EOF)
break;
@@ -132,11 +131,15 @@ usage(void)
* so you can format while you format
*/
static void
-fmtfmt(struct tm *tm, long tv_nsec)
+fmtfmt(const struct timespec *ts)
{
- char *f, ms[7];
+ struct tm *tm;
+ char *f, us[7];
+
+ if ((tm = localtime(>tv_sec)) == NULL)
+ err(1, "localtime");
- snprintf(ms, sizeof(ms), "%06ld", tv_nsec / 1000);
+ snprintf(us, sizeof(us), "%06ld", ts->tv_nsec / 1000);
strlcpy(buf, format, bufsize);
f = buf;
@@ -157,7 +160,7 @@ fmtfmt(struct tm *tm, long tv_nsec)
f += 2;
l = strlen(f);
memmove(f + 6, f, l + 1);
- memcpy(f, ms, 6);
+ memcpy(f, us, 6);
f += 6;
}
} while (*f != '\0');
Re: ts(1): make timespec-handling code more obvious
On Tue, Jul 05, 2022 at 11:08:13AM +0200, Claudio Jeker wrote:
> On Mon, Jul 04, 2022 at 05:10:05PM -0500, Scott Cheloha wrote:
> > On Mon, Jul 04, 2022 at 11:15:24PM +0200, Claudio Jeker wrote:
> > > On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> > > > Hi,
> > > >
> > > > Couple things:
> > > >
> > > > [...]
> > >
> > > 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.
> >
> > You need an "elapsed" variable to avoid overwriting "now" in the
> > -i flag case to avoid calling clock_gettime(2) twice.
> >
> > We can get rid of "utc_start" and just reuse "now" for the initial
> > value of CLOCK_REALTIME.
> >
> > How is this?
>
> How about this instead?
Looks like an improvement
The suggestion to change 'ms' to 'us' might be a good one to roll into
this changeset too.
nitpick: the changeset doesn't apply cleanly:
$ cat ts.c.rej
@@ -40,8 +40,7 @@
{
int iflag, mflag, sflag;
int ch, prev;
- struct timespec roff, start, now;
- struct tm *tm;
+ struct timespec start, now, utc_offset, ts;
int clock = CLOCK_REALTIME;
if (pledge("stdio", NULL) == -1)
Re: ts(1): make timespec-handling code more obvious
On Mon, Jul 04, 2022 at 05:10:05PM -0500, Scott Cheloha wrote:
> On Mon, Jul 04, 2022 at 11:15:24PM +0200, Claudio Jeker wrote:
> > On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> > > Hi,
> > >
> > > Couple things:
> > >
> > > [...]
> >
> > 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.
>
> You need an "elapsed" variable to avoid overwriting "now" in the
> -i flag case to avoid calling clock_gettime(2) twice.
>
> We can get rid of "utc_start" and just reuse "now" for the initial
> value of CLOCK_REALTIME.
>
> How is this?
How about this instead?
--
:wq Claudio
Index: ts.c
===
RCS file: /cvs/src/usr.bin/ts/ts.c,v
retrieving revision 1.5
diff -u -p -r1.5 ts.c
--- ts.c3 Jul 2022 15:06:06 - 1.5
+++ ts.c5 Jul 2022 08:49:55 -
@@ -32,7 +32,7 @@ static char *buf;
static char*outbuf;
static size_t bufsize;
-static void fmtfmt(struct tm *, long);
+static void fmtfmt(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 start, now, utc_offset, ts;
int clock = CLOCK_REALTIME;
if (pledge("stdio", NULL) == -1)
@@ -93,22 +92,22 @@ main(int argc, char *argv[])
if (setenv("TZ", "UTC", 1) == -1)
err(1, "setenv UTC");
- clock_gettime(CLOCK_REALTIME, );
clock_gettime(clock, );
- timespecsub(, , );
+ clock_gettime(CLOCK_REALTIME, _offset);
+ timespecsub(_offset, , _offset);
for (prev = '\n'; (ch = getchar()) != EOF; prev = ch) {
if (prev == '\n') {
clock_gettime(clock, );
if (iflag || sflag)
- timespecsub(, , );
+ timespecsub(, , );
else if (mflag)
- timespecadd(, , );
+ timespecadd(, _offset, );
+ else
+ ts = now;
+ fmtfmt();
if (iflag)
- clock_gettime(clock, );
- if ((tm = localtime(_sec)) == NULL)
- err(1, "localtime");
- fmtfmt(tm, now.tv_nsec);
+ start = now;
}
if (putchar(ch) == EOF)
break;
@@ -132,11 +131,15 @@ usage(void)
* so you can format while you format
*/
static void
-fmtfmt(struct tm *tm, long tv_nsec)
+fmtfmt(const struct timespec *ts)
{
+ struct tm *tm;
char *f, ms[7];
- snprintf(ms, sizeof(ms), "%06ld", tv_nsec / 1000);
+ if ((tm = localtime(>tv_sec)) == NULL)
+ err(1, "localtime");
+
+ snprintf(ms, sizeof(ms), "%06ld", ts->tv_nsec / 1000);
strlcpy(buf, format, bufsize);
f = buf;
Re: ts(1): make timespec-handling code more obvious
On Mon, Jul 04, 2022 at 11:15:24PM +0200, Claudio Jeker wrote:
> On Mon, Jul 04, 2022 at 01:28:12PM -0500, Scott Cheloha wrote:
> > Hi,
> >
> > Couple things:
> >
> > [...]
>
> 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.
You need an "elapsed" variable to avoid overwriting "now" in the
-i flag case to avoid calling clock_gettime(2) twice.
We can get rid of "utc_start" and just reuse "now" for the initial
value of CLOCK_REALTIME.
How is this?
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.c4 Jul 2022 17:29:03 - 1.6
+++ ts.c4 Jul 2022 22:06:56 -
@@ -32,7 +32,7 @@ static char *buf;
static char*outbuf;
static size_t bufsize;
-static void fmtfmt(struct tm *, long);
+static void fmtfmt(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;
clockid_t clock = CLOCK_REALTIME;
if (pledge("stdio", NULL) == -1)
@@ -93,22 +92,25 @@ main(int argc, char *argv[])
if (setenv("TZ", "UTC", 1) == -1)
err(1, "setenv UTC");
- clock_gettime(CLOCK_REALTIME, );
clock_gettime(clock, );
- timespecsub(, , );
+ if (mflag) {
+ clock_gettime(CLOCK_REALTIME, );
+ timespecsub(, , _offset);
+ }
for (prev = '\n'; (ch = getchar()) != EOF; prev = ch) {
if (prev == '\n') {
clock_gettime(clock, );
- if (iflag || sflag)
- timespecsub(, , );
- else if (mflag)
- timespecadd(, , );
- if (iflag)
- clock_gettime(clock, );
- if ((tm = localtime(_sec)) == NULL)
- err(1, "localtime");
- fmtfmt(tm, now.tv_nsec);
+ if (iflag || sflag) {
+ timespecsub(, , );
+ if (iflag)
+ start = now;
+ fmtfmt();
+ } else {
+ if (mflag)
+ timespecadd(, _offset, );
+ fmtfmt();
+ }
}
if (putchar(ch) == EOF)
break;
@@ -132,11 +134,15 @@ usage(void)
* so you can format while you format
*/
static void
-fmtfmt(struct tm *tm, long tv_nsec)
+fmtfmt(const struct timespec *ts)
{
+ struct tm *tm;
char *f, ms[7];
- snprintf(ms, sizeof(ms), "%06ld", tv_nsec / 1000);
+ if ((tm = localtime(>tv_sec)) == NULL)
+ err(1, "localtime");
+
+ snprintf(ms, sizeof(ms), "%06ld", ts->tv_nsec / 1000);
strlcpy(buf, format, bufsize);
f = buf;
Re: ts(1): make timespec-handling code more obvious
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 - 1.6
> +++ ts.c 4 Jul 2022 18:17:20 -
> @@ -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 __deadusage(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, );
> - clock_gettime(clock, );
> - timespecsub(, , );
> + if (clock != CLOCK_REALTIME) {
> + clock_gettime(clock, );
> + if (mflag) {
> + clock_gettime(CLOCK_REALTIME, _start);
> + timespecsub(_start, , _offset);
> + }
> + } else
> + clock_gettime(CLOCK_REALTIME, );
>
> for (prev = '\n'; (ch = getchar()) != EOF; prev = ch) {
> if (prev == '\n') {
> clock_gettime(clock, );
> - if (iflag || sflag)
> - timespecsub(, , );
> - else if (mflag)
> - timespecadd(, , );
> - if (iflag)
> - clock_gettime(clock, );
> - if ((tm = localtime(_sec)) == NULL)
> - err(1, "localtime");
> - fmtfmt(tm, now.tv_nsec);
> + if (iflag || sflag) {
> + timespecsub(, , );
> + if (iflag)
> + start = now;
> + print_timestamp();
> + } else {
> + if (mflag)
> + timespecadd(, _offset, );
> + print_timestamp();
> + }
> }
> 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(>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
