Re: [PATCH] Inline useless nested function mi_arena.

[OndÅej BÃlka <neleai at seznam dot cz>]

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.