bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code

[Giorgos Keramidas]

The following reply was made to PR bin/71623; it has been noted by GNATS.

From: Giorgos Keramidas <keramida at freebsd.org>
To: Dan Lukes <dan at obluda.cz>
Cc: bug-followup at 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 <dan at obluda.cz> 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 <stdio.h>.  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.