This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Remove nested function in elf/dl-lookup.c.
- From: "Carlos O'Donell" <carlos at redhat dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>
- Cc: GNU C Library <libc-alpha at sourceware dot org>, OndÅej BÃlka <neleai at seznam dot cz>, Roland McGrath <roland at hack dot frob dot com>
- Date: Fri, 28 Feb 2014 18:15:53 -0500
- Subject: Re: [PATCH] Remove nested function in elf/dl-lookup.c.
- Authentication-results: sourceware.org; auth=none
- References: <52FBDC5F dot 4010908 at redhat dot com> <Pine dot LNX dot 4 dot 64 dot 1402122049000 dot 12558 at digraph dot polyomino dot org dot uk> <52FBE74C dot 4070805 at redhat dot com>
On 02/12/2014 04:27 PM, Carlos O'Donell wrote:
> On 02/12/2014 03:50 PM, Joseph S. Myers wrote:
>> This patch loses the call to ELF_MACHINE_SYM_NO_MATCH that was previously
>> in check_match, and also needs to update the comment on the default
>> definition of ELF_MACHINE_SYM_NO_MATCH to reflect that check_match is no
>> longer a nested function.
>
> I have thrown away the existing refactoring and started again,
> and then compared my new version to the old version to ensure
> that previous testing would still be valid.
>
> This should catch any changes since the last time I refactored.
> I had included Ondrej's changes but had not noticed your changes.
> Git had given me a merge warning for Ondrej's changes for
> __glibc_*likely, but not yours, perhaps I did something wrong there.
>
> Either way I've rested the following and it looks good.
>
> OK?
>
> v2
> - Fixed ELF_MACHINE_SYM_NO_MATCH comment.
> - Added ELF_MACHINE_SYM_NO_MATCH check.
Given the objections to nested functions I've gone ahead and
checked this in after additional regression testing on more
targets including ppc64le.
I checked in version 2 which includes the fixups
commit 8e25d1e7721d8078d8925e325799740dd53a5801
Author: Carlos O'Donell <carlos@redhat.com>
Date: Fri Feb 28 18:11:06 2014 -0500
Promote do_lookup_x:check_match to a full function.
While it may be argued that nested functions make the resulting
code easier to read, or worse to read the following two bugs
make it difficult to debug:
Bug 8300 - no local symbol information within nested or nesting
procedures
https://sourceware.org/bugzilla/show_bug.cgi?id=8300
Bug 53927 - wrong value for DW_AT_static_link
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53927
Until these are fixed I've made check_match a full function.
After they are fixed we can resume arguing about the merits
of nested functions on readability and maintenance.
Cheers,
Carlos.