Date: Mon, 13 Sep 2004 00:40:24 GMT From: Dan Lukes <dan@obluda.cz> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/71623: [PATCH] cleanup of the usr.sbin/pcvt code Message-ID: <200409130040.i8D0eOdR055855@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/71623; it has been noted by GNATS.
From: Dan Lukes <dan@obluda.cz>
To: Giorgos Keramidas <keramida@freebsd.org>
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 ?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200409130040.i8D0eOdR055855>