Date: Sun, 12 Sep 2004 20:11:01 GMT From: Giorgos Keramidas <keramida@freebsd.org> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code Message-ID: <200409122011.i8CKB1uj028221@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/71623; it has been noted by GNATS.
From: Giorgos Keramidas <keramida@freebsd.org>
To: Dan Lukes <dan@obluda.cz>
Cc: bug-followup@freebsd.org
Subject: Re: bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code
Date: Sun, 12 Sep 2004 22:21:05 +0300
On 2004-09-12 04:37, Dan Lukes <dan@obluda.cz> wrote:
> *** usr.sbin/pcvt/cursor/cursor.c.ORIG Sat Jan 20 00:11:18 2001
> --- usr.sbin/pcvt/cursor/cursor.c Sat Sep 11 20:32:47 2004
> int start = -1;
> int end = -1;
> int dflag = -1;
> - char *device;
> ! char *device = device; /* "init" to avoid "might be used unitialized" warning */
The comment is not needed. What's the value of `device' that you use as
the initialization here? It's very likely to contain garbage data.
IMHO it would be better if you initialized it to something sensible like
NULL.
> + void
> usage()
This is still not a prototype of the function. If the warning you're
trying to fix is the lack of a proper prototype for the function the
canonical fix should be to add the missing prototype.
> + static void usage(void);
Like this ;-)
> +
> + int
> main(argc,argv)
> int argc;
> char *argv[];
Converting all the functions to post-K&R syntax is also a good thing, but
it's more a style change than a bugfix so a more experienced src-committer
should help you separate stylistic from bugfix or ofunctional changes.
> ! char *filename;
> ! char *filename = filename; /* "init" to avoid "might be used uninitialized" warning */
Please don't. See the comments about `device' I wrote earlier.
> + return(0);
A space after return to indicate that it's not a function call would be
nice here.
> - char *device;
> + char *device = device; /* "init" to avoid "might be used uninitialized" warning */
Please use NULL here too, remove the comment and make sure that the value
of device is checked for NULL before derefenced. It's probably more work,
but the above `fix' is not really a fix. Just a hack to force GCC to
shuttup about a very legitimate warning.
> - char *map;
> + char *map = map; /* "init" to suppress "might be used unitialised" warning */
Ditto.
> for(i = 0; s[i]; i++)
> {
> - if(((s[i] > 0x20) && (s[i] <= 0x7e)) || ((s[i] >= 0xa0) && (s[i] <= 0xff)))
> + if( isprint(s[i]) )
This one is actually nice. I'm not sure if there was a good reason why it
was initially checked as it was, but the change seems to be good if it also
buys us the ability to work with any locale setup ;-)
> addr_str = standard[j].addr;
> - if(new_str = kgetstr(cap, &addr_str))
> + if((new_str = kgetstr(cap, &addr_str)))
If you do add the extra parentheses perhaps it would also be nice to add an
explicit check for a NULL return value. I know that it's a typical idiom
to write:
if (pointer) { ... }
but spelling out that this is supposed to be non-NULL is good IMHO.
> - char *filename;
> + char *filename = filename; /* "init" to avoid "might be used uninitialised" warning */
NULL here too.
> - char *device;
> + char *device = device; /* "init" to avoid "might be used uninitialised" warning */
Ditto.
> ! if(cp = findname(idx))
> ! if((cp = findname(idx)))
Spacing is not very good here and an explicit check against NULL would be
nice to have:
if ((cp = filename(idx)) != NULL)
> ! if (kbdc < 32) printf(" %s", ckeytab[(short int)kbdc].csymbol);
Does the value really have to be a (short int) here? Wouldn't an (int) be
fine too? If not, it would be nice to have a comment nearby explaining why
a plain (int) is not ok.
> else {
> - while (c = *s++) choice = 10 * choice + c - '0';
> + while ((c = *s++)) choice = 10 * choice + c - '0';
Please add an explicit check against '\0' too if you add the extra
parentheses here.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200409122011.i8CKB1uj028221>