From owner-freebsd-bugs@FreeBSD.ORG Mon Sep 13 00:40:24 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 9C1BE16A4CE for ; Mon, 13 Sep 2004 00:40:24 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7B46A43D49 for ; Mon, 13 Sep 2004 00:40:24 +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 i8D0eO5i055858 for ; Mon, 13 Sep 2004 00:40:24 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.12.11/8.12.11/Submit) id i8D0eOdR055855; Mon, 13 Sep 2004 00:40:24 GMT (envelope-from gnats) Date: Mon, 13 Sep 2004 00:40:24 GMT Message-Id: <200409130040.i8D0eOdR055855@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Dan Lukes 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: Dan Lukes List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Sep 2004 00:40:24 -0000 The following reply was made to PR bin/71623; it has been noted by GNATS. From: Dan Lukes To: Giorgos Keramidas Cc: bug-followup@freebsd.org Subject: Re: bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code Date: Mon, 13 Sep 2004 02:30:43 +0200 (CEST) On Sun, 12 Sep 2004, Giorgos Keramidas wrote: >> ! char *device = device; /* "init" to avoid "might be used unitialized" warning */ >> ! char *filename = filename; /* "init" to avoid "might be used uninitialized" 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. It's misunderstanding. I recommend two diferent type of patches. In the first case, the variable IS used uninitialized. In that case, I recommend initialisation of a value and classify PR as "serious". It's not the case here. In this case, the variable IS NOT used uninitialised, so I try to eliminate "false positive" warning only. The "device=device" assigment during declaration is the trick doing this. True initialisation is unnecesarry here and is waste of memory and cpu cycles. >> + 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. It isn't really fix, because fix is NOT necesarry here. Device variable is correctly initialised before use. You got it - it's just a hack. I'm trying hack to force GCC shutup about false warning. >> + 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); The warning in question is "implicit declaration of ..." I'm agree, 'static' should be used. >> + >> + 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. It's not simple style change. Here the "return type defaults to int" warning has been issued, so returning type has been added explicitly. No other changes has recommended. I'm trying to eliminate pile of warnings, not to correct style of the code.. >> + return(0); > > A space after return to indicate that it's not a function call would be > nice here. True. >> - 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 ;-) The original reason was "comparsion always true" warning caused by (s[i] <= 0xff) > >> 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 True. >> ! 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. Because kbdc is type of char. Short int should be sufficient for char. Or not ?