throopw@xyzzy.UUCP (Wayne A. Throop) (12/16/88)
In the code fragment posted in this line of conversation, there was
one "style fault" that could actually qualify as a bug under draft
X3J11, and which therefore I felt ought to be pointed out.
Now when I got done with the posting, I found I had picked on four
more points of style in the code fragment, and my posting had ended up
looking like a hatchet job rather than a warning of a nonportable
practice. Well, rather than pare down my nits to the most important
one, I'm posting the whole critique. I hope folks won't take this as
the hatchet job it superficially resembles, but will take in the way
it was intended: an important warning followed by four stylistic
afterthoughts that I really and truely think result in code of
"superior style". Happy coding, and if you feel I've been too harsh
despite the disclaimer, feel free to email me and vent your spleen.
The fragment in question is:
void strupr( s )
char s[];
{
int p = 0;
while ( s[p] != NULL ) {
s[p] = toupper( s[p] );
p++;
}
} /* strupr */
My comments, in decreasing order of urgency (or increasing order of
pickiness) are these.
First, comparing a character to NULL is stylistically wrong in K&R C,
since NULL is to be used in pointer contexts (just as FALSE or NO or
whatnot is to be used in "boolean" contexts). In draft X3J11 C, this
is even a potential error, since NULL can legitimately be defined as
((void *)0), in which case an implementation (I think) is allowed to
diagnose an error in the above comparison.
Second, the loop control is scattered over the body of a while, which is
exactly what the for loop was invented for.
Third, the function might as well return s instead of void, so that
it can be slipped unobtrusively into expressions instead of forcing
separate statements or awkward expressions.
Fourth, it isn't indicated that ctype.h ought to be included.
(This is, of course, fairly obvious, and may have been omitted on
purpose...)
Fifth, the object of this excersize is somewhat more naturally cast in
terms of pointers rather than subscripting in C. Two marginal
justifications for this are that first it is more natural to step
along an array once by pointers, and second compilers with few or no
optimizing smarts will generate better code for the pointer case on
most (but not all) architectures. (This, of course, is a really picky,
obnoxiously finicky nit.)
Applying all of these, we get the "improved" fragment (using prototypes):
#include <ctype.h>
char *strupr( char *s ) {
register char *p;
for( p=s; *p != '\0'; p+=1 )
*p = toupper( *p );
return( s );
}
Note that some might prefer the condition in the for loop to be
*p != 0
or just
*p
(In fact, I prefer just *p myself, but most folk seem to prefer the
explicit comparison to a character value.)
--
"Leave him alone. He is already neutralized."
"I don't want neutral. I want dead."
--- exchange in "Die Hard"
--
Wayne Throop <the-known-world>!mcnc!rti!xyzzy!throopwmsb@sq.uucp (Mark Brader) (12/20/88)
Wayne A. Throop (throopw@xyzzy.UUCP) made, in <2404@xyzzy.UUCP>, some criticisms of a certain piece of code including these lines: > > int p = 0; > > while ( s[p] != NULL ) { The last of these was: > Fifth, the object of this exercise is somewhat more naturally cast in > terms of pointers rather than subscripting in C. Wayne went on to cite two admittedly minor justifications for this. And I'd like to add another: avoiding the use of "int" for the subscript! Remember that "int" is allowed to be as small as 16 bits. It is quite conceivable that s[] has more than 32767 elements in it, so p can overflow, followed by God-knows-what (or as the X3J11 people say, by "undefined behavior"). A similar problem can exist if we make p "unsigned"; in this case it would be an infinite loop. On the other hand, some compilers wouldn't allow the object's dimension to exceed an int's range, so declaring p as "long" could be wasteful. If we use pointers instead of subscripts to step through the array, no such problem can exist. A pointer that points to the beginning of an array is guaranteed to be incrementable to the end of the array (plus one more time, in Draft ANSI C); and there is only one size of pointer variable (in true C, anyway): the right size. Mark Brader "Those who do not understand UNIX SoftQuad Inc., Toronto are condemned to repeat it." utzoo!sq!msb, msb@sq.com -- Henry Spencer