This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] remove nested function hack_digit


> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]