From owner-freebsd-bugs@FreeBSD.ORG Mon Sep 13 06:20:26 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 6E91B16A4CF for ; Mon, 13 Sep 2004 06:20:26 +0000 (GMT) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id C7D3743D49 for ; Mon, 13 Sep 2004 06:20:23 +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 i8D6KNQH020671 for ; Mon, 13 Sep 2004 06:20:23 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.12.11/8.12.11/Submit) id i8D6KNFK020670; Mon, 13 Sep 2004 06:20:23 GMT (envelope-from gnats) Date: Mon, 13 Sep 2004 06:20:23 GMT Message-Id: <200409130620.i8D6KNFK020670@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: Mon, 13 Sep 2004 06:20:26 -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: Mon, 13 Sep 2004 09:15:49 +0300 On 2004-09-13 02:30, Dan Lukes wrote: > 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. It's a hack. Hacks are very rarely nice and should be evaluated carefully for their risk vs. merit ratio. In this case the risk is IMHO great and the merit is not more than that provided by a standard initialization to NULL. If the code below this point changes in the future and `filename' is REALLY used before a proper initialization is done, this hack will hide the bug. Shutting up the compiler is mostly ok but, at least, we shouldn't introduce the potential for hiding future problems. Initializing a pointer to "some" value, even though this value is based on garbage data, is not guaranteed to work on all architectures or implementations. The C standard says that trying to set the value of a pointer variable to anything else except NULL and the address ranges of valid objects can cause a "trap" and triggers implementation-specific behavior. It would be terrible to discover that after porting FreeBSD to an exotic platform the porters would have to hunt for bogus initializations and "fix" them for us. > 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.. The latter (fixing the style) is also a good reason to change the source :) > >>! 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 ? IIRC, the range of values for `char' is always within the bounds of the range for `short'. So yes, short int is sufficient. I just meant that a cast to a plain `int' would probably be more preferrable, like for example many functions of . Apparently there's no special reason why this value should be `short' and not `int', so keeping the existing practice of using an `int' for holding a `char' value is what my suggestion was about.