Re: [PATCH] Convert old prototype.

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Sat, 8 Jun 2013, Ondrej Bilka wrote:

> Continuing cleanup we now focus on converting old prototype to new.
> 
> I now have patch that does formating rigth, see:
> 
> http://kam.mff.cuni.cz/~ondra/convert_old_prototype.patch
> 
> It could be applied as it is but it is not exhaustive as I needed 
> to exclude several special cases like macro to modify function name.

Being non-exhaustive is fine.  It's best to try to deal with the simple 
cases first and then deal with the trickier ones separately in smaller 
patches, possibly by hand, rather than trying to make one patch cover 
everything.

My observations on this patch are:

* In crypt/speeds.c, function clearmem, and crypt/ufc.c, function main, 
you modify not just the initial part of the function definition with 
return type, name and parameters, but also change the indentation of the 
line with the function's opening brace.  I don't think this patch should 
do that; I think it should limit itself to the part of the definition 
before the opening brace (which means that the line with the opening brace 
should only be affected if the opening brace is, before the patch on the 
same line as a parameter declaration).  Of course in GNU style the opening 
brace shouldn't be indented, but it doesn't make sense to change its 
indentation on its own without keeping the rest of the function matching.

* You have an incorrect change in 
ports/sysdeps/unix/sysv/linux/hppa/sysdep.c; I presume your cleanup 
program had a false positive there.  Likewise sysdeps/mach/hurd/ptrace.c.

* You are changing intl/plural.c, but that's a generated file.  Some of 
the old-style definitions are in plural.y (rather than in Bison's skeleton 
parser), so you can change that and regenerate plural.c, but it's probably 
best to leave that for a separate patch dealing with trickier cases and 
just exclude plural.c from the initial bulk conversion.

* In backtrace.c, for example, your new prototype says "void * *array", 
but the space in "* *" is bogus.  The old-style definition had correct 
spacing.  I would say it's fine for this change to fix incorrect spacing 
in the parameter declarations (e.g. where you change "_IO_FILE* fp" to the 
correct "_IO_FILE *fp"), but not to introduce incorrect spacing.

> I could make this patch exhaustive if I could do 
> make and make check with enabled
>  -Wold-style-definition
> , my attempts ended in getting errors from -Werror.
> 
> 
> then I could parse that output and run my script on these lines.

I don't think -Werror is really what you want for parsing output anyway.  
Once the basic patch for easy cases is in, I'd suggest enabling 
-Wold-style-definition without -Werror, running a (non-parallel) build and 
looking at the warnings in the output.  That way a single build can catch 
all of the old-style definitions for a particular configuration rather 
than stopping on the first error.  Once x86 and x86_64 are clean of such 
warnings, other people can fix their architectures at leisure.  We've 
never worked out a strategy for enabling -Werror anyway (what to do about 
the various residual hard-to-fix warnings in some files on some 
architectures).

Have you verified on at least one platform that the patch causes no 
changes to the instruction sequence generated for glibc's installed shared 
libraries?

-- 
Joseph S. Myers
joseph@codesourcery.com