From owner-freebsd-bugs@FreeBSD.ORG Sun Sep 12 20:11:01 2004 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id B97F916A4DC for ; Sun, 12 Sep 2004 20:11:01 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9EECB43D2F for ; Sun, 12 Sep 2004 20:11:01 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.12.11/8.12.11) with ESMTP id i8CKB1E1028222 for ; Sun, 12 Sep 2004 20:11:01 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.12.11/8.12.11/Submit) id i8CKB1uj028221; Sun, 12 Sep 2004 20:11:01 GMT (envelope-from gnats) Date: Sun, 12 Sep 2004 20:11:01 GMT Message-Id: <200409122011.i8CKB1uj028221@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Giorgos Keramidas Subject: Re: bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list Reply-To: Giorgos Keramidas List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 12 Sep 2004 20:11:02 -0000 The following reply was made to PR bin/71623; it has been noted by GNATS. From: Giorgos Keramidas To: Dan Lukes 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 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.