This is the mail archive of the mailing list for the glibc project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Inline useless nested function mi_arena.

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.

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.

> 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?

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.

> 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?


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]