[Prev][Next][Index]
Re: Should I CAPITALIZE const identifiers?
-
Subject: Re: Should I CAPITALIZE const identifiers?
-
From: ok@cs.rmit.edu.au (Richard A. O'Keefe)
-
Date: 1 Dec 1995 20:26:25 -0600
-
Approved: clc@solutions.solon.com
-
Newsgroups: comp.lang.c.moderated
-
Organization: Comp Sci, RMIT, Melbourne, Australia
-
References: <49fi36$asm@solutions.solon.com> <49gbgl$ee6@solutions.solon.com>
-
Sender: clc@solutions.solon.com
I have received some E-mail critical of Lint. With the utmost possible
respect, I suggest that anyone who finds hundreds of warnings from Lint
and discards the tool because only 1/8th of them are real is not exercising
reasonable care.
I thought that it might be a good idea to give a demonstration of what
lint can do for you. So I picked up the very first program currently
available in comp.sources.unix.
1. The program is 'more', slightly revised. It has been around for quite
a while.
2. The makefile does not contain an entry for 'lint'.
3. This is happening live. 3:18PM. About to start. For fairness,
let's put it through the compiler first, and fix what the compiler
moans about. 3:15pm make all.
more.c no warnings
morefile.c no warnings
magic.c missing header <a.out.h>
Reason for problem: this is a Sun running Solaris 2.4, which uses
ELF-32 object files, not the old and lovable a.out format. Look at
source code. Ahah! There's #ifndef SOLARIS, but the README file
said nothing about that. Patch the makefile and add comments.
3:23 try make all again.
magic.c no warnings
link re_exec() and re_comp() not found.
This system _does_ have them, and the README mentions them, but
there were no clues about where to find them. Now I've got a problem.
Use gcc -O2 -Wall (which the Makefile doesn't) to find more errors,
or use /usr/ucb/cc in order to access this library. Patch and
comment the Makefile again. make clean; make all. Oh CURSE!
more.c:1229: va_start: argument mismatch
Can I compile with gcc and link with /usr/ucb/cc? 3:28 pm try it.
gcc -O -DSOLARIS -c *.c
/usr/ucb/cc *.o -ltermcap
Whew! Got through the compiler.
Now, let's see what `glint` does.
#!/bin/sh
# NAME glint
# AUTHOR Richard A. O'Keefe
# PURPOSE Use GCC to get as much checking as possible.
# I want -O2 -Wall to catch uninitialised variables, but that means
# I can't use -fsyntax-only. Hence the use of -c.
exec gcc -c -ansi -pedantic -O2 -Wall -Dgets=DONT_USE_GETS -Dlint\
-Wtraditional -Wshadow -Wpointer-arith -Wnested-externs -Winline $@
Hokay. 3:30pm. glint -DSOLARIS *.c >&ERRS; wc ERRS
62 lines of warnings from gcc. (Why would anyone use gcc and NOT
use gcc -O2 -Wall?)
Problem 1. morefile.h has
static inline off_t Ftell(struct mfile *mp) {
return mp->file_pos;
}
This is good stuff, but it ain't (standard) C. The inline keyword
is C++ and GNU C. (Sun's C compiler is happy to expand functions
like this inline without the keyword. There are three occurrences
of this. Now there is some code in there that is _supposed_ to
protect against this. It says
#if !defined(__GNUC__) && !defined(inline)
#define inline
#endif
This has a number of problems. First, there are compilers other than
GNU C that support inline, so it shouldn't be switched off without
cause. Second, as I just found out the hard way, it doesn't work if
you _are_ using GNU C. What I'm going to do is follow the advice in
the GCC manual page and change this to
#ifndef Inline
#define Inline
#endif
and in the Makefile I'll put -DInline=__inline__.
3:39pm. Rerun glint. wc ERRS now -> 56 lines. That's better.
Problem 2. 10 instances of "declaration of `ch' shadows global
declaration." This warning often indicates real errors. Check 'em.
I note that 'char ch' is declared global in more.c, but it's only
used in more.c. It should have been static. First shadowing is
in a prototype, but while prototypes _can_ have argument names it
is a dangerous practice (there are better methods of documenting
functions for human understanding). So's the 2nd. The 3rd is
rather more interesting. main() contains the declaration
register char ch;
But it ALSO contains the use
ch = Getc(f)
with ch later being tested against EOF. I can't BELIEVE this!
Getc() in this program can return anything that getc() can return,
WHICH MEANS THE RESULT WILL NOT FIT IN A CHAR (257 possible values
but a char only holds 256). If my input contains a y-umlaut, more
will think it's an EOF. This is a _classic_ C bug, it's made clear
in C Traps and Pitfalls. Change the declaration to
register int ch; /* any character or EOF */
and note that the program is likely to have serious 8-bit problems.
4th occurrence harmless. Looking at the last occurrence I note that
CARET (this thing: ^) has been mispelled CARAT (a measure of weight
for gemstones) Fixed that, and fixed an ASCII-ism.
Deleted the global declaration of 'ch' as it was in fact unused.
In fact it turned out to be declared _twice_ in the same file.
3:55pm Rerun glint. 45 lines of warnings.
There are a couple of "suggest parentheses around assignment used
as truth value"; I don't greatly care for the extra parentheses,
but I _love_ getting a clean compile. Fixing the first gave me a
chance to fix a 7-bit-ism. (Found lots more, oh _dear_.)
4:09 pm. Rerun glint. 37 lines of warnings.
Problem 3. Trouble with fileno(). The problem here is that fileno()
is not a C function, it's a _POSIX_ one. There's an official way to
get POSIX things from standard headers, so add
#define _POSIX_C_SOURCE 1
at the top of each .c file. Fixed.
4:13 pm. Rerun glint. 39 lines of warnings. Oops.
Look in /usr/include/sys/stat.h, it should have worked.
Hang on a minute, gcc used
/opt/gnu/.lib/gcc-lib/sparc-sun-solaris2.3/2.6.3/include/sys/stat.h
instead, and THAT doesn't define quite the same things as the system
header. DRAT! Use #define _XOPEN_SOURCE. That fixed it.
Problem 3 was all about porting between different versions of UNIX.
There is a warning sign here: POSIX.1 and the _POSIX_C_SOURCE and
even _XOPEN_SOURCE feature test macros are not new. It didn't need
to be this hard. What else is lurking there?
4:22 pm. Rerun glint. 34 lines of warnings.
Problem 4: 8 occurrences of "warning, variable xxxx might be clobbered
by `longjmp' or `vfork'". Sure enough, main() has THREE calls to
setjmp()! This is a bad sign. They all set the same jump buffer,
so longjmp(restore) will have three different meanings at run time.
Not just a long distance goto, but a long distance _varying_ goto.
main() really should be restructured, but I'm just producing an
example.
The key thing is that if you've got variables that must survive a
setjmp()/longjmp(), they MUST be declared 'volatile'. The best
thing would be to restructure the code, but tacking volatile on
will correct the symptom.
Two of the warnings refer to a variable 'mp' which does not occur
in the source code. It apparently results from in-line expansion.
It points to a variable that's now volatile, so those functions
need changing too. This was such a pain, restructuring might have
been easier. BUT THE PROBLEM WAS REAL!
4:31 pm. Rerun glint. Oops. I wrote
register volatile struct mfile *f;
instead of
register struct mfile *volatile f;
Now down to 25 lines.
Problem 5. 'rcsid' undeclared in 'argscan'. The code is wrong.
It says
#ifndef lint
static char rcsid[] = "...";
#endif
but the string is _used_ whether lint is defined or not. Remove
ifndef.
4:44 pm rerun glint.
Problem 6. 'retval might be used uninitialised' in command().
Oddly enough the main structure here is
done = 0;
while (1) {
...
if (done) break;
}
What's going on is that there are places where the code wants to
break from the loop, but can't because there's a switch() in the
way, which of course BCPL solved by using 'endcase' for switches.
Changed the loop for
for (retval = done = 0; !done; ) {
}
Also moved 'break' out of the ret() macro so that the cases were
_visibly_ properly terminated.
4:57 pm rerun glint. 19 lines of warnings.
Problem 7. Implicit declaration of open().
First thought: but the file #includes <unistd.h>.
Second thought: oh yes, but open() is in <fcntl.h>
This matters because open() is a varargs function and needs a
prototype. Add the #include.
5:00 pm. rerun glint. 17 lines of warnings.
They fall into two main classes: `storage size of "win" not known'
and `comparison between signed and unsigned'.
The first class was caused by the addition of _POSIX_C_SOURCE and
_XOPEN_SOURCE. Now I have to #define __EXTENSIONS__ as well.
Basically what this is telling me (as if I were still in any doubt)
is that the program will need some work to use POSIX.1 facilities
instead of BSD facilities. Yes, this is the compiler being picky,
but it is telling me something I need to know.
5:06 pm rerun glint 11 lines of warnings
Two classes: implicit declaration of ioctl. That's odd, because
man ioctl() says it's in unistd.h and that _is_ included.
In /usr/include/unistd.h I find
#if (!defined(_POSIX_C_SOURCE) && !defined(+XOPEN_SOURCE)) || \
defined(__EXTENSIONS__)
extern int ioctl(int, int, ...);
#endif
but __EXTENSIONS__ _is_ defined, so .... I wonder if it's
/opt/gnu/lib/gcc-lib/sparc-sun-solaris2.3/2.6.3/include
again? OH THE SODS! They've changed it to
#if !defined(_POSIX_C_SOURCE)
extern int ioctl(int, int, ...);
#endif
I could do without this kind of "help" from gcc, I really could.
(Perhaps it's a change from Solaris 2.3 to Solaris 2.4 and gcc is
still using old headers. I can do without that too.)
Change to
glint -I/usr/include {remaining options as before}
5:15pm. Rerun glint with new options. 7 lines containing 5 warnings.
Problem 9. Comparing a char with an unsigned char. More 7-bit muck.
Changed two variables (one from char, one from int) to unsigned char.
Changed two variables in another function to fix same problem.
5:26 pm. Rerun glint. At last. NO MORE WARNINGS FROM GCC!
4. Ok. Now I've been fair. GCC _did_ find a lot of problems.
What will lint do?
5:29 run lint. 166 lines of warnings.
Problem 10. Lint doesn't like while (1). Neither do I.
<debatable style>
Problem 11. Lint doesn't like if ((x = y)). Gcc didn't like it
when it reqad if (x = y). I don't much like this either; this
is a case where x and y would have had type bool had it existed.
Change it to if (0 == (x = y)) .
<debatable style>
Problem 11. Two fallthroughs in switches(). This is often enough
wrong that I am very glad Lint tells me about it. Fix 'em.
It turned out that one of them needed a 'break'.
<good one, lint!>
Problem 12. "Semantics of < change in ANSI C." Since this is a
pre-ANSI program spiffed up a bit I should check.
It's a piece of code I added to fix another problem, and a
cast will certainly make my intentions clear.
<doesn't count>
Problem 13. Four functions have a 'sig' argument they don't use.
Looks as though they might be signal handlers. While adding
/*ARGSUSED*/ to them, notice that they do things that are not
kosher selon the C standard, and some use stdio which is not
_quite_ proper in the POSIX standard (read() and write() yes).
Notice that the code consistently uses
write(2, "string", integer_literal)
instead of
write(2, "string", sizeof "string" - 1)
which is a maintenance problem, but that can wait.
<debatable style but real problems in neighbourhood>
Problem 14. Pointer case may result in improper alignment.
The code is
unsigned long elfmagic;
char *elfbytes = ELFMAG;
elfmagic = *((unsigned long *)elfbytes;
where
#define ELFMAG "\177ELF"
and lint is perfectly correct. There is no guarantee whatsoever
that this string will be allocated on a longword boundary; the
code may indeed crash. Rewrote it to
Elf32_Word elfmagic;
memcpy(&elfmagic, ELFMAG, sizeof elfmagic);
which is free of such problems.
<real problem found by lint but not gcc>
Problem 15. Assignment causes implicit narrowing.
Eliminated one instance by unfolding unique call to function.
Eliminated one instance by giving a variable the right type.
Eliminated one instance by giving a function the right type.
One instance remains:
int c; char *p; ...
c = Getc(...); ... if (c == EOF) { ...} else { .. *p = c; ... }
I often come across this. It _is_ a bogus warning, because lint
doesn't know that the range of Getc() is {EOF} U {0..UCHAR_MAX}
and can't be told, and isn't that bright about constant propagation
anyway. Shut lint up by adding a cast.
<3 cases of wrong type; 1 bogus warning>
Problem 16. Suspicious comparison of unsigned with 0.
Tests of the form [unsigned] > 0 are valid, but the change from
K&R semantics to ANSI semantics has not been easy or painless,
and on the whole I'd rather be given the chance to review.
Rephrased to eliminate warning.
<debatble standard transition warning>
Fixing these problems left me with 146 lines of lint output.
Now it's 6:05pm.
Problem 17. morefile.h defines three static inline functions,
which are only used by two of the files that include it.
If they were macros lint would never know.
<program structure bad, but no actual mistake involved>
Problem 18. 10 "name used but not defined" warnings.
The reason for this is simple: Sun haven't bothered to supply
a complete set of lint libraries. They didn't even bother to
supply a lint library for <curses.h>, so I sent them one.
There are two libraries here: termcap (BSD-specific, considered
obsolete) and the BSD regular expression library (ditto), and
they don't have lint backup.
<porting problem>
The underlying problem here is that the program uses BSD facilities
and doesn't have code for System V. That's what Lint is really
telling me here.
Problem 19. 3 "name declared but never used or defined" warnings.
I'd like to switch this one off; it never helps me.
Two of them have the same underlying problem as problem 18.
One of them is Sun not keeping their lint library up to date.
(What the heck _is_ 'confstr' anyway?)
Problem 20. Function returns a value which is always ignored:
memcpy -- not a problem
setjmp -- not a problem
Ungetc -- OOPS, it can fail! [private]
kill -- OOPS, it can fail!
close -- OOPS, it can fail! (but what can you do?)
execv -- OOPS, it can fail!
sleep -- not a problem
write -- OOPS, it can fail!
Fseek -- OOPS, it can fail! [private]
strcpy -- not a problem
open -- OOPS, it can fail!
fclose -- OOPS, it can fail! (but what can you do?)
fflush -- OOPS, it can fail!
sprintf -- OOPS, it can fail! (but is used safely)
fputs -- OOPS, it can fail!
puts -- OOPS, it can fail!
tcsetattr -- OOPS, it can fail (maybe it won't)
or returns a value which is sometimes ignored:
pr -- it _can_ fail but doesn't say!
signal -- OOPS, it can fail! [private]
printf -- OOPS, it can fail!
putchar -- OOPS, it can fail!
fseek -- OOPS, it can fail!
tcgetattr -- OOPS, it can fail.!
These can be classified into several groups:
functions whose result can always be safely ignored
memcpy, setjmp, sleep, strcpy, pr
execv -- if it returns, it certainly failed
functions whose result can be ignored if you checked
sprintf (format visible and correct, buffer big enough)
signal (all calls correct)
Ungetc (I _think_)
functions that can report an error, but what can you do?
close, fclose
-- well, you can _report_ the error for a start
functions whose result must be checked lest output be lost
write, fflush, fputs, puts, printf, putchar
-- fseek and Fseek would belong here, but this program uses
-- them only on input files
other functions that can go wrong
kill, open.
Having checked, I find that open() _is_ used in a way that can fail,
and is even _likely_ to fail, but the result is not checked.
From: where!
Problem 20. Two function argument inconsistencies.
crypt(const char *, const char *) in unistd.h
crypt(char *, char *) in lint library
Looks like another case of Sun not keeping their lint library
up to date.
Problem 21. Declared global, could be static. 101 instances.
This program is divided into several files. It _could_ benefit
from having these things declared static. In fact good style in
C is to declare everything static at top level unless it absolutely
_has_ to be visible outside. If I were going to do much work with
this program, I'd do something about this.
<debatable style>
In any case, this warning can be switched off, so let's give
lint the command line arguments -m and -x.
6:32 remake lint. 27 lines of information (discounting the file
names shown as checking proceeds and blank lines). This comes
to
3 lines static functions in a header not called by includer
14 lines due to vendor's lint library out of date or incomplete
9 lines warning about function results being ignored.
Basically, Lint has told me
- this program is not 8-bit clean.
[It has helped me fix some of the problems, but not all.]
- this program came from another environment and has not
been upgraded to full POSIX compliance.
- there is a pointer alignment problem which COULD have crashed
the program (now fixed)
- a number of vital library functions that can and in practice do
fail have their result ignored, so the program can for example
fail to notice that it is writing to a full or off-line disc,
or that there is no terminal.
- the vendor's lint library does not support BSD packages that
are considered obsolete in Solaris, and has errors and omissions
of its own.
These are things that GCC did NOT tell me, even with lots of warning
messages on. It _could_ have told me about most of them. It happens
that this program uses headers well and is thorough about prototypes,
so the kind of inter-module inconsistency checking that lint does so
well was not needed. But it still found important things!
5. Now, what will LC-Lint see? It will be hampered by the fact that it
doesn't have Larch specifications for anything but the standard
library, but maybe, just maybe, it will find something.
It's 6:41 pm. Time to move the files over to the machine where I keep
LC-lint and try it. I had a phone call from an encyclopedia salesman,
(no, we can't let you try the CD-ROM edition, it's too much trouble
to install and if we do that people try looking at it...)
It's 7:05 PM. LC-lint produces 573 lines of output.
The first warning is about a redefinition of ungetc. The declaration
it sees is
extern int ungetc(int, FILE *);
but it's expecting
extern int ungetc(char, FILE *);
which is wrong. Ok, bug report to LC-Lint maintainers. There are
a couple of other functions it has wrong. Also reported.
There are 121 "Return value (type xxx) ignored" messages.
It's complaining about the same things that Lint found, but it shows
you the actual calls instead of just saying which functions. This
is a great help. I don't know why Lint can't do that.
There are 4 "parameter not used" messages; the same things that
Lint found. (I have asked the lclint maintainers to accept and
processs the /*ARGSUSED*/ comment. I don't know what they'll decide.)
There are 109 lines of warnings because in its default mode lclint
distinguishes bool from int. I can switch that off with +boolint.
There are 83 warnings that the code applies "->" to something that
is not a struct or union pointer; this is lclint being really strict
about abstract data types. For reasons I won't go into here having
to do with compiling on one machine and lclinting on another, I
gave lclint already preprocessed code to check. If that _had_ been
hand-written code I _would_ have been violating the abstraction.
There is one warning
Array fetch using non-integer, char: (__ctype+1)[*s].
I warned you this was already preprocessed code. What this means
is that the code has
isdigit(*s)
where char *s;
Seems like an obvious thing to do, doesn't it, and cc and gcc both
let it slip through, BUT IT'S BROKEN! The program has no control
over what's in that string; there could be characters with the
high bit set, in which case they'll be negative. This is more
all-the-world-uses-ASCII code (false on UNIX, OS/2, Mac, and DOS).
There's an uninitialised variable warning; the code is sufficiently
twisted (3 calls to setjmp) that I'm not sure if it's real or not.
Setting it to an empty string is harmless, so I've done that.
There's an uninitialised variable warning in code of the form
... foo(...) {
static int flag;
...
if (flag && ...) {
Admittedly a style warning, but why _not_ help the reader by doing
static int flag = 0;
lclint is picky about the distinction between char and int; this is
a *very* useful feature if you ever intend to convert your C code
to C++, which is even stricter about that than lclint.
lclint found
while ((c = Getc(f)) != '\n')
if (c == EOF)
where c is declared as char. Somehow cc, gcc, and lint had missed it.
Another bug fixed!
I did have to wade through a lot of stuff to get there. It paid off
though.
Version 2.0 of lclint is supposed to be coming out soon.
Walter Briscoe wrote:
>I hope my mind is not closed. I have started to look at LC lint with a
>view to porting it to a 16 bit environment. Unfortunately, the
>documentationm is in Postscript and I lack an engine for that.
>Can anyone point me at filters for Postscript?
Ghostview is what I use. But I have just learned that there is an
HTML version of the documentation in
http://www.sds.lcs.mit.edu/~evs/userguide/userguide.html
I'm viewing it with NCSA Mosaic right now. No problems that I can see.
--
"conventional orthography is ... a near optimal system for the
lexical representation of English words." Chomsky & Halle, S.P.E.
Richard A. O'Keefe; http://www.cs.rmit.edu.au/~ok; RMIT Comp.Sci.