Cleaning up X server warnings
So I was sitting in the Narita airport with a couple of other free software developers merging X server patches. One of the developers was looking over my shoulder while the X server was building and casually commented on the number of warnings generated by the compiler.
I felt like I had invited someone into my house without cleaning for months -- embarrassed and ashamed that we'd let the code devolve into this state.
Of course, we've got excuses -- the X server code base is one of the oldest pieces of regularly used free software in existence. It was started before ANSI-C was codified. No function prototypes, no 'const', no 'void *', no enums or stdint.h. There may be a few developers out there who remember those days (fondly, of course), but I think most of us are glad that our favorite systems language has gained a lot of compile-time checking in the last 25 years.
We've spent time in the past adding function prototypes and cleaning up other warnings, but there was never a point at which the X server compiled without any warnings. More recently, we've added a pile of new warning flags when compiling the X server which only served to increase the number of warnings dramatically.
The current situation
With the master branch of the X server and released versions of the dependencies, we generate 1047 warnings in the default build.
-Wcast-qual considered chatty
The GCC flag, -Wcast-qual, complains when you cast a pointer and change the 'const' qualifier status. A very common thing for the X server to do is declare pointers as 'const' to mark them as immutable once assigned. Often, the relevant data is actually constructed once at startup in allocated memory and stored to the data structure. During server reset, that needs to be freed, but free doesn't take a const pointer, so we cast to (void *), which -Wcast-qual then complains about. Loudly.
Of the 1047 warnings, 380 of them are generated by this one warning flag. I've gone ahead and just disabled it in util/macros for now.
String constants are a pain
The X server uses string constants to initialize defaults for font paths, configuration options, font names along with a host of other things. These end up getting stored in variables that can also take allocated storage. I've gone ahead and declared the relevant objects as const and then fixed the code to suit.
I don't have a count of the number of warnings these changes fixed; they were scattered across dozens of X server directories, and I was fixing one directory at a time, but probably more than half of the remaining warnings were of this form.
And a host of other warnings
Fixing the rest of the warnings was mostly a matter of stepping through them one at a time and actually adjusting the code. Shadowed declarations, unused values, redundant declarations and missing printf attributes were probably the bulk of them though.
Changes to external modules
Instead of just hacking the X server code, I've created patches for other modules where necessary to fix the problems in the “right” place.
- proto/fontsproto. Declares FontPathElement names as 'const char *'
- mesa/drm. Adds 'printf' attribute to the debug_print function
- util/macros. Removes -Wcast-qual from the default warning set.
Getting the bits
In case it wasn't clear, the X server build now generates zero warnings on my machine.
I'm hoping that this will also be true for other people. Patches are available at:
xserver - git://people.freedesktop.org/~keithp/xserver warning-fixes
fontsproto - git://people.freedesktop.org/~keithp/fontsproto fontsproto-next
mesa/drm - git://people.freedesktop.org/~keithp/drm warning-fixes
util/macros - already upstream on master
Keeping our house clean
Of course, these patches are all waiting until 1.15 ships so that we don't accidentally break something important. However, once they're merged, I'll be bouncing any patches which generate warnings on my system, and if other people find warnings when they build, I'll expect them to send patches as well.
Now to go collect the tea cups in my office and get them washed along with the breakfast dishes so I won't be embarrassed if some free software developers show up for lunch today.