This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Inline useless nested function mi_arena.
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Carlos O'Donell <carlos at redhat dot com>
- Cc: Siddhesh Poyarekar <siddhesh at redhat dot com>, libc-alpha at sourceware dot org
- Date: Mon, 26 May 2014 14:20:43 +0200
- Subject: Re: [PATCH] Inline useless nested function mi_arena.
- Authentication-results: sourceware.org; auth=none
- References: <20140226214924 dot GA10204 at domone dot podge> <20140520154152 dot GH14500 at spoyarek dot pnq dot redhat dot com> <537BC8FF dot 3040101 at redhat dot com> <20140521025059 dot GL14500 at spoyarek dot pnq dot redhat dot com> <53832BF6 dot 9050900 at redhat dot com>
On Mon, May 26, 2014 at 07:56:38AM -0400, Carlos O'Donell wrote:
> On 05/20/2014 10:50 PM, Siddhesh Poyarekar wrote:
> > On Tue, May 20, 2014 at 05:28:31PM -0400, Carlos O'Donell wrote:
> >> The inlined mi_arena already has 3-deep control structures and inlining
> >> it makes them 4-deep control structures.
> >>
> >> None of this code is in the hot path, so therefore my guiding principle
> >> is to make this easier to debug and maintain.
> >
> > We agree on the guiding principle, it's just that our opinions on what
> > is easier to maintain and debug is different.
>
> That happens :-)
>
> > I prefer the inlining to a separate function for three reasons:
> >
> > 1. The separate function is called only once, so inlining does not
> > result in duplication of code.
>
> I agree.
>
> > 2. The 4-deep nesting does not make the code unreadable, because the
> > code flow otherwise is quite simple. There are no arbitrary jumps
> > out of loops and the conditions are very simple.
>
> I went back and reviewed cyclomatic complexity, which applies here
> as a correlative indicator of maintenance cost.
>
> The original function already by my rough count has a McCabe cyclomatic
> complexity of 15, and any function >10 is complex to understand and
> difficult for testing to go through all the if/else and loops conditions.
>
> Merging that into the existing function results in a function that is
> even harder to understand and test.
>
> Cyclomatic complexity is the only metric I can bring to bear here in
> order to provide some objectivity to our "feelings" of what's readable
> or unreadable.
>
It is just a heuristic. What is more important is not to break code flow
and hide what is not important. This function is conceptualy simple,
just iterate arenas and print stats. Trying to break function at random
points is not a solution to mainatainability.