xserver janitorial work

[Matthieu Herrb]

Greg KH wrote:
> I'm starting to go through the xserver tree and cleaning up a variety of
> minor "janitorial" type stuff in order to get a bit more familar with
> the code base.  I was starting to just clean up the obviously wrong
> compiler warnings, and running 'sparse' on the tree when I ran into some
> code that has old, K&R style function declarations.
> 
> An example of this would be:
> 	Bool
> 	mfbRealizeFont( pscr, pFont)
> 	    ScreenPtr   pscr;
> 	    FontPtr     pFont;
> 	{
> 	    return (TRUE);
> 	}
> 
> Tools like 'sparse' choke hard on this kind of C code, so does anyone
> object to me cleaning functions like this up to be "proper" C code?
> This example would then look like:
> 	Bool
> 	mfbRealizeFont(ScreenPtr pscr, FontPtr pFont)
> 	{
> 	    return (TRUE);
> 	}
> 

Yes, but be extra-careful to not change a function's signature by 
accident. You probably already know it, but there are at least 2 cases 
that can hurt if not careful enough:

- parameters that get promoted to int or double in K&R style
(they should ihmo be declared as int or double in "ansi" declaration, in 
order to be compatible with old code)

- the order of actual parameters and parameter declaration is not 
necessarly the same in K&R, if you use the parameter declaration to 
generate the new parameter list, it may end up in the wrong order.

Since there are not so many people running the X test suite (or any 
extensive testing with good code coverage) on a regular basis, some of 
the error that can be introduced here make take weeks or month to be 
detected, and become hard to locate and fix.

Another point that is worth noticing is that the X server uses quite a 
lot of pointer to functions of incomplete types, with several different 
actual function types. Converting this code generally means adding 
unions or other rather ugly stuff. These should be reviewed on a case by 
case basis to find the less ugly way to handle it.

> 
> Also, I see a lot of places where #ifdefs are used within .c files and
> even within functions that can easily be fixed with proper header
> definitions to keep the .c code clean and provide no extra code
> generated.
> 
> A (made up) example of that would be something like:
> 
> #ifdef FOO_FEATURE
> static void foo_feature(int baz)
> {
> 	do_something_interesting(baz);
> }
> #endif
> 
> ....
> 
> void big_foo(int bar)
> {
> 	....
> #ifdef FOO_FEATURE
> 	if (some_test_is_true()) {
> 		foo_feature(bar);
> 	}
> #endif
> 	.....
> }
> 
> That can be rewritten to be:
> 
> #ifdef FOO_FEATURE
> void foo_feature(int baz)
> {
> 	do_something_interesting(baz);
> }
> #else
> static inline foo_feature(int baz) { }
> #endif
> 
> ....
> 
> void big_foo(int bar)
> {
> 	....
> 	if (some_test_is_true()) {
> 		foo_feature(bar);
> 	}
> 	.....
> }
> 
> 
> Any objections to these kinds of cleanups?

I'm not sure if this really improves readability in practice. Someone 
looking on the snipset above  may later think "hey, but foo_feature() is 
optionnal, let's add back a #ifdef around this code as in my case it 
will save some bytes". Ad Lib...

-- 
Matthieu Herrb