Re: [PATCH] Inline useless nested function mi_arena.

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

On Mon, May 26, 2014 at 05:52:10PM +0530, Siddhesh Poyarekar wrote:
> On Mon, May 26, 2014 at 07:56:38AM -0400, Carlos O'Donell wrote:
> > 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.
> > 
> > The simpler the function the less time it takes to review,
> > so while you might argue "it's simple enough", that equates to some fixed
> > amount of time to read the code. If the function was *simpler* the
> > review and reading would be *faster* allowing anyone to go through the
> > code quickly.
> 
> OK, I just have a higher threshold for cyclomatic complexity ;)
> 
> But on a serious note, I don't disagree that the code is complicated,
> I only claim that the function breakout looks ugly.
> 
> > > 3. Making a separate non-inline function would require passing of a
> > >    lot of context to the function for just one call - you would end up
> > >    with a function call with 10 arguments.  That IMO is much more
> > >    unwieldy to maintain than the simple inline bit.
> > 
> > Why does this make it unwieldly to maintain?
> 
> Because (as you guessed) it makes for a complicated internal API.
> 
> > It makes for a complex internal API, I agree, but if you carefully
> > use `const' it also draws a pretty clean line between what you can
> > and can't change and makes easily visible how to refactor both functions
> > to make things simpler.
> 
> There's not a lot to mark as const.  In fact, everything is modified
> in the function AFAICT, so you'll essentially end up passing
> references to all counters into the function.
> 
> > > That said, the best option for what I am planning to do with this
> > > function (add mmap stats and add an option to produce JSON) would be
> > > to collate all the statistics into a struct and pass to struct to a
> > > printer function when we're done collecting data.  However, that is
> > > even more code rework, so it might be suitable for a separate pass.
> > 
> > In which case I still advocate a distinct function with `const' arguments
> > where appropriate instead of inlining. Since anything else is a waste of
> > time because we want JSON stats and we're reworking the code?
> 
> OK, lets just get the function out first.  I don't think it's
> important enough to argue over right now since we're going to rework
> the code in later passes anyway.  Ondrej, could you please do a second
> version of the patch with the function just popped out and the global
> counters passed as references?
> 
I do not write patches that in my opinion harm project. Let Carlos write
it if he want that.