bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code
keramida at freebsd.org
Sun Sep 12 23:20:26 PDT 2004
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
> 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.
More information about the freebsd-bugs