xserver janitorial work

[Greg KH]

On Thu, Jun 08, 2006 at 01:03:54AM +0200, Matthieu Herrb wrote:
> 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)

How would this happen?  With variables that have no explicit "type"?

> - 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.

The compiler _should_ catch this now with a proper function prototype
(I'm also trying to clean up these and the ever popular "just stick an
extern foo(int); in this other file" issues so this is caught at build
time.)

> 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.

'git bisect' will help out with that :)

But yes, I understand the issues and will be very careful to try to not
mess up.

> 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.

Yeah, I've noticed that, and for now will stay away from that issue...

> >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...

Heh, true (for those that don't realize it, there is no savings, the
compiler took out that logic already).

I guess I'm just used to "cleaner" .c code, and IMHO, no #ifdefs in the
middle of a function is a good idea.

It looks like I have enough other good "real" cleanups to do first so
I'll hold off on doing this for now.

thanks,

greg k-h