[PATCH] rcs void casts
Hi tech,
there are a few void casts in rcs. But I have a question about that.
Are these casts really necessary? I've read that the compiler warns, because of
unused variables.
But no compiler warnings about that on amd64.
That's why I just added this small diff, in order to get feedback if the casts
are necessary or not.
fritjof
Index: rlog.c
===
RCS file: /cvs/src/usr.bin/rcs/rlog.c,v
retrieving revision 1.67
diff -u -p -r1.67 rlog.c
--- rlog.c 7 Jan 2014 14:08:16 - 1.67
+++ rlog.c 7 May 2014 20:24:37 -
@@ -517,7 +517,7 @@ rlog_rev_print(struct rcs_delta *rdp)
fmt = %Y/%m/%d %H:%M:%S;
}
- (void)strftime(timeb, sizeof(timeb), fmt, t);
+ strftime(timeb, sizeof(timeb), fmt, t);
printf(\ndate: %s; author: %s; state: %s;, timeb, rdp-rd_author,
rdp-rd_state);
@@ -556,7 +556,7 @@ rlog_rev_print(struct rcs_delta *rdp)
TAILQ_FOREACH(rb, (rdp-rd_branches), rb_list) {
RCSNUM *branch;
branch = rcsnum_revtobr(rb-rb_num);
- (void)rcsnum_tostr(branch, numb, sizeof(numb));
+ rcsnum_tostr(branch, numb, sizeof(numb));
printf( %s;, numb);
rcsnum_free(branch);
}
Re: [PATCH] rcs void casts
Hi Fritjof,
Fritjof Bornebusch wrote on Wed, May 07, 2014 at 10:32:05PM +0200:
there are a few void casts in rcs. But I have a question about that.
Are these casts really necessary?
No, they are not necessary.
I've read that the compiler warns, because of unused variables.
But no compiler warnings about that on amd64.
Usually, you don't choose your style to please static analysis tools -
there are too many of them, of too varying quality and signal-to-noise
ratio, to please them all.
Rather, it makes sense to tune static analysis tools, including
compilers, to warn about dangerous style, but to not warn about style
that is merely a matter of taste.
That said, for functions that usually require checking of return
values, the void cast can be used to indicate this call was audited,
and ignoring the return value is considered safe, either because
this specific call cannot actually fail, or because failure does
not need explicit handling at this point. Typical functions where
this makes sense are snprintf(3), strlcpy(3), strlcat(3). So i
clearly wouldn't remove the cast from strftime() unless you have
reason to assume that no proper audit was done, or this is broken
and needs fixing. In this case, the buffer is large enough, so
the cast should stay.
On the other hand, on functions that hardly ever require checking
the return code, casting to void is pointless, so i tend to
remove such casts when editing nearby code. A typical example
is (void)close(fd); which is rarely more helpful than
just close(fd);.
In this case, the rcsnum_tostr() return value can't be used for error
checking in the first place, that function calls errx(3) on failure,
so when editing nearby code, i'd probably remove that cast. But it
doesn't warrant a commit on it's own imho.
Yours,
Ingo
Index: rlog.c
===
RCS file: /cvs/src/usr.bin/rcs/rlog.c,v
retrieving revision 1.67
diff -u -p -r1.67 rlog.c
--- rlog.c 7 Jan 2014 14:08:16 - 1.67
+++ rlog.c 7 May 2014 20:24:37 -
@@ -517,7 +517,7 @@ rlog_rev_print(struct rcs_delta *rdp)
fmt = %Y/%m/%d %H:%M:%S;
}
- (void)strftime(timeb, sizeof(timeb), fmt, t);
+ strftime(timeb, sizeof(timeb), fmt, t);
printf(\ndate: %s; author: %s; state: %s;, timeb, rdp-rd_author,
rdp-rd_state);
@@ -556,7 +556,7 @@ rlog_rev_print(struct rcs_delta *rdp)
TAILQ_FOREACH(rb, (rdp-rd_branches), rb_list) {
RCSNUM *branch;
branch = rcsnum_revtobr(rb-rb_num);
- (void)rcsnum_tostr(branch, numb, sizeof(numb));
+ rcsnum_tostr(branch, numb, sizeof(numb));
printf( %s;, numb);
rcsnum_free(branch);
}
Re: [PATCH] rcs void casts
On Wed, May 07, 2014 at 10:58:03PM +0200, Ingo Schwarze wrote:
Hi Fritjof,
Fritjof Bornebusch wrote on Wed, May 07, 2014 at 10:32:05PM +0200:
there are a few void casts in rcs. But I have a question about that.
Are these casts really necessary?
No, they are not necessary.
I've read that the compiler warns, because of unused variables.
But no compiler warnings about that on amd64.
Usually, you don't choose your style to please static analysis tools -
there are too many of them, of too varying quality and signal-to-noise
ratio, to please them all.
Rather, it makes sense to tune static analysis tools, including
compilers, to warn about dangerous style, but to not warn about style
that is merely a matter of taste.
That said, for functions that usually require checking of return
values, the void cast can be used to indicate this call was audited,
and ignoring the return value is considered safe, either because
this specific call cannot actually fail, or because failure does
not need explicit handling at this point. Typical functions where
this makes sense are snprintf(3), strlcpy(3), strlcat(3). So i
clearly wouldn't remove the cast from strftime() unless you have
reason to assume that no proper audit was done, or this is broken
and needs fixing. In this case, the buffer is large enough, so
the cast should stay.
On the other hand, on functions that hardly ever require checking
the return code, casting to void is pointless, so i tend to
remove such casts when editing nearby code. A typical example
is (void)close(fd); which is rarely more helpful than
just close(fd);.
In this case, the rcsnum_tostr() return value can't be used for error
checking in the first place, that function calls errx(3) on failure,
so when editing nearby code, i'd probably remove that cast. But it
doesn't warrant a commit on it's own imho.
Yours,
Ingo
I see, thank you for the explanations.
It was just for me to make it clear when (void) is used.
fritjof
Index: rlog.c
===
RCS file: /cvs/src/usr.bin/rcs/rlog.c,v
retrieving revision 1.67
diff -u -p -r1.67 rlog.c
--- rlog.c 7 Jan 2014 14:08:16 - 1.67
+++ rlog.c 7 May 2014 20:24:37 -
@@ -517,7 +517,7 @@ rlog_rev_print(struct rcs_delta *rdp)
fmt = %Y/%m/%d %H:%M:%S;
}
- (void)strftime(timeb, sizeof(timeb), fmt, t);
+ strftime(timeb, sizeof(timeb), fmt, t);
printf(\ndate: %s; author: %s; state: %s;, timeb,
rdp-rd_author,
rdp-rd_state);
@@ -556,7 +556,7 @@ rlog_rev_print(struct rcs_delta *rdp)
TAILQ_FOREACH(rb, (rdp-rd_branches), rb_list) {
RCSNUM *branch;
branch = rcsnum_revtobr(rb-rb_num);
- (void)rcsnum_tostr(branch, numb, sizeof(numb));
+ rcsnum_tostr(branch, numb, sizeof(numb));
printf( %s;, numb);
rcsnum_free(branch);
}
