VerySillyMUD: Cleaning up the Warnings

October 11, 2015. Series [ VerySillyMUD ]

This post is part of my “VerySillyMUD” series, chronicling an attempt to refactor an old MUD into working condition [ 1 ].

In our last episode, we discovered a bug that the compiler had actually warned us about, but which we ignored because we were trying to just fix errors and get on with running the game. Lesson learned here; time to go clean up all those warnings. As there are a lot of them, and they are likely going to be somewhat repetitive, I will just highlight the particularly interesting ones here. However, you can see the full set of cleanups here.

The first thing we want to do is to recompile all the object files to see what the warnings are. I was about to make clean except that the Makefile doesn’t seem to have a clean target! Let’s fix that. Then, let’s make sure we don’t make this mistake again by adding the -Werror flag to the CFLAGS variable to treat compiler warnings as errors. Now, the first type of error we encounter is:

Basically, we have not explicitly declared a prototype for the atoi library function, nor have we #included one. This is simply a matter of adding the right system include file in the case of C library functions. Next:

When you are not explicit about your braces, and you have nested if statements, you can have unintentional paths through your code when an else gets associated with the wrong if. We can correct these by adding explicit braces, taking indentation in the source code as a hint as to what was probably intended. Next we find that adding in some missing #include files created some conflicts:

A global variable reboot is used to track whether the game is supposed to restart itself after an intentional, non-crash shutdown. This conflicts with the reboot system call that actually reboots the whole machine! We can handle this by renaming the variable, taking care to find all locations of it in other files.

We also fixed some cases where functions were declared with a non-void return type yet there was no explicit return statement. As we saw in a previous episode, we can fix these by changing the declaration to return void if no callers ever check for a return value. Our next problem involves the type of one of the arguments passed to the bind() system call:

A glance at the code shows that sa is declared as a struct sockaddr_in (an Internet socket address), but bind wants a pointer to a struct sockaddr. Here’s one of those cases where C expects you to cast one struct to another with the same prefix, for as much confusion as that might cause. This is a standard C TCP/IP socket programming idiom, however. The next error complains of an implicit function definition, but is a little more involved:

With a little judicious grepping, we find that CAN_SEE_OBJ is declared in utility.c, where we see:

Ugh. This apparently was a macro at one point but was redefined as a function, although someone conveniently left the old macro definition in here for us, but compiled out with #if 0. At any rate, we can clean that out and then add the missing prototype. For the next error, the compiler helpfully gives us options about how to fix it:

We can fix this by explicitly comparing the value of the assignment to a null character ('\0') instead. I also encountered some type errors showing up mismatches between a format string and an argument:

Again, the compiler happily provided a useful suggestion in these cases to update the format string. I think this is an area where the C compilers have improved since I was doing a lot of C programming; I’m not sure they would always catch these for me in the past. But I might also be misremembering!

Next, we have a couple spots where the developers wanted to be extra sure about order of operations:

In this case, a quick perusal of the code suggests that this was indeed meant to be a comparison and not an assignment, so we can just get rid of the extra parens.

Now, at one point I ran into some type errors surrounding function pointers:

Ugh, function pointers. I can never get the typing right on these and always have to look up how to do it. In this case, it looks like the programmers just got lazy, calling all the function pointers void (*)(), but we can propagate the correct types through the code instead.

And then I ran into this error:

Hmm, taking a look at the code shows:

Ok, I can see the problem, which is essentially some kind of nested sprintf calling. This needs to be refactored, but there’s no way this function is clear enough to me to refactor it without unit tests. Guess we’ll have to get that going in the next post.

[ 1 ] SillyMUD was a derivative of DikuMUD, which was originally created by Sebastian Hammer, Michael Seifert, Hans Henrik Stærfeldt, Tom Madsen, and Katja Nyboe.