scs@adam.mit.edu (Steve Summit) (03/02/91)
In article <2488@njitgw.njit.edu> yxz1684@hertz.njit.edu (Ying Zhu ee) writes: >The following is a small program from "Mastering C pointers" by Robert J. >Traister. I am confusing with the pointer here. Since the r[100] is an >automatic variable, after calling the "combine" the r[100] will be deallocated. >Then the p will point to an unsafe place. You are the innocent victim of a cruel hoax. You assumed that a book entitled "Mastering C Pointers" might have been written by someone who was, himself, a master in the use of C pointers. Evidently this is anything but the case. You would be well advised to throw this book very far away; clearly you already know more about correct pointer use than does its author. I can't quite believe that the code sample presented was actually published, but my friendly neighborhood technical bookstore has closed for the evening, so I'll have to take the complainant's word for it. In fact, returning a pointer to auto storage which is no longer valid is only the worst of the code's several problems. Let's take another look at it, shall we? (Put your rubber gloves on first, and preferably a clothespin on your nose, too.) >main() >{ > char a[10], b[10], *p, *combine(); > > strcpy( a, "horse" ); > strcpy( b, "fly" ); > > p=combine(a, b); > > printf("%s\n", p ); >} The strings a and b are initialized rather oddly, and main() neither returns a value nor calls exit(), but these are minor. The "combine" routine is far more interesting: >char *combine(s,t) >char *s, *t; >{ > int x,y; > char r[100]; > > strcpy(r,s); > y=strlen(r); > for ( x=y; *t != '\0'; ++x ) > r[x] = *t++; > r[x]='\0'; > > return(r); >} First of all, the returned result array r is, as mentioned, an automatic array, a pointer to which should not be returned. The array's size is fixed, and would be exceeded by easily imaginable calling patterns. Overflow is not checked. None of the variables have very good names. A variable named "x" is used as an index; this is somewhat unusual, especially since this is an algorithm (copying characters hither and yon) which no C programmer worthy of the title would code using an array subscript. A pointer solution is far more likely, especially in a book entitled "Mastering C Pointers." (Perhaps a second example in the book showed how to convert the routine to use all pointers.) A variable named "y" is used once, as the length of the first of the two strings to be combined; it is unneded, as the call to strlen might as well appear in the loop initialization expression. It is unfortunate that the first string has to be traversed a second time (by the call to strlen) to determine its length, when that could have been determined as a side effect of the initial copy, but the side effect is buried inside strcpy. The use of standard library routines to do part of a job is generally commendable, though (as long as the job is worth doing -- see below), so we'll let this point pass. We could rewrite the routine taking the above observations into account, but wait a minute. The middle four lines of the "combine" routine could be replaced with a call to the standard library routine strcat. Clearly, calling upon standard library routines is not considered objectionable in this code, as strcpy and strlen have been used already. Before rewriting "combine" to use only strcpy and strcat, however, we might note that "combine" is almost identical to strcat, except that strcat appends the second string to the first and returns its first argument, while "combine" tries to build a third string. But that's the part it gets most wrong. Rather than hauling in this useless "combine" routine, the author would have been much better advised to demonstrate simple implementations of the standard library routines such as strlen, strcpy, and strcat. (It would be wise to call them my_strlen, my_strcpy, and my_strcat, so that the student would not abuse a reserved identifier when compiling his own version, and to permit regression testing.) I have often though that it would be wonderful if Kernighan and Plaugher could come out with a third edition of their classic Elements of Programming Style, incorporating C examples instead of PL/I and Fortran. I thought that perhaps there were not enough published bad examples to serve as the base for such a book. Silly me. Mastering C Pointers would clearly be a mother lode. Whoops, someone else just tried to answer the original question. Let's take a look: In article <1991Mar1.235912.25133@ux1.cso.uiuc.edu> gordon@osiris.cso.uiuc.edu (John Gordon) writes: >> p=combine(a, b); > ^^^^^^^^^^^^^^^^ -> This is wrong. 'p' has not yet been correctly > initialized. pointers need to be initialized > before you can set them equal to something. Mr. Gordon needs to learn more about pointers and initialization. The only thing wrong about p's initialization here is that the pointer value that "combine" returns is bogus; this is combine's fault, not p's. Yes, pointers need to be initialized before they can be used, but what better way to initialize one than by sticking it on the left hand side of an equals sign and putting an expression of type "pointer to char" on the right? The suggestion that we > could change p's declaration to char p[256] and > then do a strcpy(p, combine(a, b)); . wouldn't hurt, although it would make the identifier "p" an array, not a pointer (see the FAQ!), which is a bit misleading. (It's also silly, in this case, since "combine" was trying to return a pointer to valid storage, although no more silly than the original code's declaring a[] and b[] and strcpying into them.) Steve Summit scs@adam.mit.edu