This is the mail archive of the libc-alpha@sourceware.org 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 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?

Thanks,
Siddhesh

Attachment: pgpTkxiC707IV.pgp
Description: PGP signature


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