lkml.org 
[lkml]   [2009]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [GIT PULL] percpu fixes for 2.6.32-rc6
Hello, Ingo.

11/12/2009 08:07 PM, Ingo Molnar wrote:
> Well, the pcpu_alloc() function is 115 lines which is a bit long. It
> does 2-3 things while a function should try to do one thing.

I agree for low level / utility functions but for top level functions
which direct the flow of the whole logic, I usually prefer to put them
flat. To me, that way things seem less obfuscated.

> Putting the reserved allocation into a separate function also makes the
> 'main' path of logic more visible and obstructed less by rare details.
>
> The indentation i pinpointed is 4 levels deep:
>
> err = "failed to extend area map of "
> "reserved chunk";
>
> which is a bit too much IMO - the code starts in the middle of the
> screen, there's barely any space to do anything meaningful.

Well, all that's there is error exit. Surrounding code segment is,

if (pcpu_extend_area_map(chunk, new_alloc) < 0) {
err = "failed to extend area map of "
"reserved chunk";
goto fail_unlock_mutex;
}

So, we might as well just do

err = "failed to extend area map of reserved chunk";
if (pcpu_extend_area_map(chunk, new_alloc) < 0)
goto fail_unlock_mutex;

> But there's other line wrap artifacts as well further down:
>
> if (pcpu_extend_area_map(chunk,
> new_alloc) < 0) {

This one is uglier and one level deeper than the previous one. The
resulting nesting was one of the reasons why I factored out
pcpu_extend_area_map() as a whole and switched on the return value but
that obfuscated locking. Although it nests quite a bit, I don't think
the loop there is too bad. It shows what it does pretty well.

> But ... there's no hard rules here and i've seen functions where 4
> levels of indentation were just ok. Anyway, i just gave you my opinion,
> and i'm definitely more on the nitpicky side of the code quality
> equilibrium, YMMV.

Indentation and code style are actually something I end up spending
quite some time on and I did think about the second one. Factoring
out without hiding locking is a bit difficult but if I rename
new_alloc to new_len, I can fit that thing onto a single line but that
would probably require renaming matching local variable in
pcpu_extend_area_map() which will end up generating unnecessary amount
of diff obfuscating the real change. At that point, I just thought we
could live with one slightly ugly line break.

So, I don't know. Pros and cons on these things depend too much on
personal tastes (and even mood at the time of writing) to form uniform
standard to follow.

Thanks.

--
tejun


\
 
 \ /
  Last update: 2009-11-12 12:31    [W:0.124 / U:0.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site