Re: code cleanup

On Thursday 29 April 2004 12:06 am, Alex Lyashkov wrote:
> > Note that the allproc_lock protects the allproc list.  W/o the
> > FOREACH_PROC macro, I can grep for 'allproc' in the source tree to find
> > all users to verify locking, etc.  With the extra macro, I now have to do
> > multiple greps.
>
> two greps is multiple ? first of FOREACH_PROC, second allproc or combine
> at one grep with two -e parameters.

Multiple means more than one, yes.  When I'm searching the tree when locking a 
structure or fields of a structure I don't usually come up with complex grep 
statements, and actually, I wouldn't find the FOREACH_FOO macro until I did 
the first grep anyway.  When you add lots of macros that do this you get a 
compounding problem.

> > When you multiple the effect with several wrapper macros, it now becomes
> > much more work to work on locking the lists of structures since you have
> > to do multiple greps to find the places to look at.  I think remembering
> > the linkages for lists is actually quite important to avoid using the
> > same linkage for multiple lists incorrectly.
>
> you wrong.
> it`s code from linux kernel, but illustrate who create more readable
> code with macros same as FORAEACH_PROC_IN_SYSTEM.
> ==========================
>         read_lock(&in_dev->lock);
>         for_primary_ifa(in_dev) {
>                 if (inet_ifa_match(a, ifa)) {
>                         if (!b || inet_ifa_match(b, ifa)) {
>                                 read_unlock(&in_dev->lock);
>                                 return 1;
>                         }
>                 }
>         } endfor_ifa(in_dev);
>         read_unlock(&in_dev->lock);
> ==========================
> where difficult for find locking problem? but using macro allow write
> more readable code.

This is not the same type of deal.  In this case, the object being locked 
'in_dev' is passed as an argument to the macro, so a grep for 'in_dev' still 
turns up this line.  A comparable change would be to have 
FOREACH_PROC_IN_SYSTEM() be 'FOREACH_PROC(p, &allprocs)' in which case grep 
for allprocs finds the line.  That actually matches the style of the other 
macros that all take two arguments: an iterator and a source of the list.  
Note that such a change would also allow the macro to be used with the 
zombprocs list as well.  I'm not convinced that

FOREACH_PROC(p, &allprocs)

is all that different from:

TAILQ_FOREACH(p, &allprocs, p_list)

though.

-- 
John Baldwin <jhb_at_FreeBSD.org>  <><  http://www.FreeBSD.org/~jhb/
"Power Users Use the Power to Serve"  =  http://www.FreeBSD.org