Re: [WIP] Remove nested function in elf/dl-lookup.c? (Nested functions considered harmful?)

["Carlos O'Donell" <carlos at redhat dot com>]

On 12/15/2013 11:41 PM, Siddhesh Poyarekar wrote:
> On Sat, Dec 14, 2013 at 10:58:46AM -0500, Carlos O'Donell wrote:
>> Is it me or does reading do_lookup_x get very very complicated
>> with the nested function check_match which has access to all
>> sorts of function local variables?
>>
>> I'm always suspicious of nested functions like this as bad
>> programming form since it complicates reading the function.
>>
>> Is anyone opposed to rewriting this with a non-nested function
>> that clearly shows which arguments are constant, and which are
>> modified?
> 
> i'd be in favour of this except...
> 
>> +/* Utility function for do_lookup_x. The caller is called with undef_name,
>> +   ref, version, flags and type_class, and those are passed as the first
>> +   five arguments. The caller then computes sym, symidx, strtab, and map
>> +   and passes them as the next four arguments. Lastly the caller passes in
>> +   versioned_sym and num_versions which are modified by check_match during
>> +   the checking process.  */
>> +static const ElfW(Sym) *
>> +check_match (const char *const undef_name,
>> +	     const ElfW(Sym) *const ref,
>> +	     const struct r_found_version *const version,
>> +	     const int flags,
>> +	     const int type_class,
>> +	     const ElfW(Sym) *const sym,
>> +	     const Elf_Symndx symidx,
>> +	     const char *const strtab,
>> +	     const struct link_map *const map,
>> +	     const ElfW(Sym) **const versioned_sym,
>> +	     int *const num_versions)
>> +{
> 
> ... that I find a function with 11 arguments a bit ugly.  Could this
> be re-factored somehow to make check_match not depend on so many
> parameters?

Maybe, maybe not, but that would be a distinct patch *after*
this initial checkin to make the function distinct.

I don't want to wrap the elements in a struct because that
breaks the optimizations used during inlining.

There are 4 computed arguments which we might just recompute
in the function... but that's still 7 arguments.

Cheers,
Carlos.