Yesterday we started tidying up the code by first cleaning up ADDSTACK and ENDSTACK in shortcut.c, before replacing them with macro definitions in proto.h. Sadly for us those macro’s don’t fix the hundreds of lines of:
stack = strchr(stack, 0);
For that we’re going to have to manually track down every sprintf and edit. But before we do, lets consider another case we’d also like to tidy up.
strcpy(stack, "some text");
stack = strchr(stack, 0);
Unlike the sprintf example, we can’t attribute this to sloppy coding. The return value for strcpy is a pointer to its destination, which in the above example is stack. Now we could replace those with sprintf, except it would be a silly idea. sprintf is an expensive function to call just for a string copy, if anything we want to convert sprintf to strcpy when its not doing any text formatting. A naive solution to this problem would look like:
char *my_strcpy(char *dest, char *src)
/* wordpress doesn't render the usual null terminator correctly, so I'll use 0 even though thats wrong to show my point */
work = strchr(dest, 0);
Which would make the code look pretty, but replaces 2 function calls with 3, and makes the code slightly slower. So onto a better example:
char *my_strcpy(char *dest, const char *src)
char *d = dest;
const char *s = src;
*d++ = *s;
while (*s++ != 0);
return d - 1;
However GNU has a library function named stpcpy which does exactly what we want. Ideally we want to use that function instead of our own when its available. To do so we first need to change our makefile. Heading to src/configure/makefiles we edit the file named linux, changing:
To now read:
Then back in our src directory, we need to “make newos” for those changes to take effect. If we now try to compile the talker, the compiler will immediately produce errors, so lets work through those.
In file included from admin2.c:24:0:
include/proto.h:439:19: error: conflicting types for strcasestr
/usr/include/string.h:371:14: note: previous declaration of strcasestr was here
GNU has a strcasestr function which now conflicts with the utility function in xstring.c. We can fix those compiler errors simply by wrapping the prototype and the code inside:
/* conflicting code is here */
One thing to note whilst we’re doing this, the PGPlus strcasestr function has a subtle bug:
Upon successful completion, strcasestr() shall return a pointer to the located string or a null pointer if the string is not found. If s2 points to a string with zero length, the function shall return s1.
Whilst PGPlus’s way of returning a null pointer is saner, any code that relies upon that will break. So we’ll change PGPlus’s default function to do the right thing. Now we check every call to strcasestr to ensure its passing a valid string, and modify it if its not.
After making those changes, our next compiler error is:
In file included from aliases.c:19:0:
include/fix.h:23:17: error: conflicting types for getitimer
/usr/include/sys/time.h:127:12: note: previous declaration of getitimer was here
And just as with the strcasestr prototype, we’ll wrap the contents of fix.h in an #ifndef…#endif block.
And one last compiler warning:
ttt.c: In function ttt_print_board:
ttt.c:94:5: warning: implicit declaration of function sprintf
ttt.c:94:5: warning: incompatible implicit declaration of built-in function sprintf
That ones our fault. When we changed ADDSTACK and ENDSTACK to be macro forms of sprintf we didn’t check every file includes the correct header. So we just need to add an include for stdio.h and our code is back to compiling cleanly, and we can return to cleaning up the code.