This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] remove nested function hack_digit
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Konstantin Serebryany <konstantin dot s dot serebryany at gmail dot com>
- Cc: Andreas Schwab <schwab at linux-m68k dot org>, GNU C Library <libc-alpha at sourceware dot org>
- Date: Fri, 26 Sep 2014 12:38:46 -0700 (PDT)
- Subject: Re: [PATCH] remove nested function hack_digit
- Authentication-results: sourceware.org; auth=none
- References: <CAGQ9bdxUJaUzz=ndu-qnhkPGAH7=m5mFKxpDag=H693TeA2ORw at mail dot gmail dot com> <87a960l9ze dot fsf at igel dot home> <CAGQ9bdyxCW-_3rLy6uLg4Vc2FPx+gUL7PChaXA4i6aKmnjGVZg at mail dot gmail dot com> <mvm38bsyppg dot fsf at hawking dot suse dot de> <CAGQ9bdya8w_OmD=1wKayhLN51H+Jqaio3RGqtATKWc6_hPgBxQ at mail dot gmail dot com> <20140922214338 dot 0D30A2C3971 at topped-with-meat dot com> <CAGQ9bdzKgTMEFM7-uL98nzqgJfOtm+U0AhzcnkgqTuqs3r_=UQ at mail dot gmail dot com> <20140922224516 dot EAC342C3971 at topped-with-meat dot com> <CAGQ9bdyOVYamtWG4L4tUp+WiL2AstZpGLRFKsSeLtLPMyv6Cow at mail dot gmail dot com> <CAGQ9bdwzvk+h=39NuJG+7TvpKu+m6j8=nGZrhidAWxiNeTuJBQ at mail dot gmail dot com>
> Here is a better formatted and tested patch that uses a single parameter.
> The generated code is nearly identical (same instructions, differences
> only in constant offsets).
This change looks mostly OK to me. It's not clear to me what you're saying
this version's generated code is identical to. If you're just saying it's
identical to the version of the patch that differs only cosmetically, then
that's not interesting. The clear comparison of the trunk code to the
patched code is what we need. It's not especially helpful to attach the
whole disassembly (we can all compile it for ourselves). What's important
is to do the careful analysis of the code differences and explain their
expected performance impact.
> +2014-09-15 Kostya Serebryany <konstantin.s.serebryany@gmail.com>
In patch submissions, the log entry should not be part of the patch.
> + * stdio-common/printf_fp.c (hack_digit): New function.
> + (___printf_fp): Remove nested function hack_digit. Call non-nested
> + function hack_digit.
Two spaces between sentences. I would have written this:
* stdio-common/printf_fp.c
(hack_digit): New function, broken out of ...
(__printf_fp): ... local function here. Update call sites.
hack_digit now takes an additional parameter that is a pointer
to a struct of the referenced locals. Those locals moved inside
the struct and references updated.
> +static wchar_t hack_digit (struct hack_digit_param *p)
> +{
The parts of the declaration before the function name get their own line,
with the function name starting the next line.
You didn't report what testing you did on this patch.
Thanks,
Roland