This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Convert old prototype.
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: OndÅej BÃlka <neleai at seznam dot cz>
- Cc: <libc-alpha at sourceware dot org>
- Date: Sat, 8 Jun 2013 15:15:37 +0000
- Subject: Re: [PATCH] Convert old prototype.
- References: <20130608104926 dot GA17305 at domone dot kolej dot mff dot cuni dot cz>
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