Re: [PATCH] decnet: remove macro-local declarations

[Julia Lawall <julia.lawall@lip6.fr>]

On Fri, 6 Nov 2015, walter harms wrote:

> +1
>
> I like this more since it is much more obvious what is done.
>
> more over we can remove a macro with only 2 users.

It's not to bad because it turns out to be pretty concise.  But I don't
agree about the 2 users.  There are 2 users for this specific definition,
but the same concept is used in 37 places, and having an inlined solution
in one place and a macro based solution in another place is not going to
be helpful.  So the decision should be made in terms of all 37 users.

julia

>
> re,
>  wh
>
>
> Am 06.11.2015 11:57, schrieb Julia Lawall:
> >>> Would it be preferable to remove the macro entirely and inline the for
> >>> loop header?
> >>
> >> Could you show me an example of how this would look exactly?
> >
> > One possible solution is below.  I moved the initialization of the nh
> > pointer inside the loop to reduce the size of the loop header.  One could
> > also inline fi->fib_nh[nhsel] where it occurs, but it seemed that that
> > would make quite long expressions.
> >
> > julia
> >
> > diff --git a/net/decnet/dn_table.c b/net/decnet/dn_table.c
> > index 1540b50..509ae82 100644
> > --- a/net/decnet/dn_table.c
> > +++ b/net/decnet/dn_table.c
> > @@ -60,11 +60,6 @@ struct dn_hash
> >
> >  #define dz_key_0(key)		((key).datum = 0)
> >
> > -#define for_nexthops(fi) { int nhsel; const struct dn_fib_nh *nh;\
> > -	for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)
> > -
> > -#define endfor_nexthops(fi) }
> > -
> >  #define DN_MAX_DIVISOR 1024
> >  #define DN_S_ZOMBIE 1
> >  #define DN_S_ACCESSED 2
> > @@ -227,7 +222,7 @@ static struct dn_zone *dn_new_zone(struct dn_hash *table, int z)
> >  static int dn_fib_nh_match(struct rtmsg *r, struct nlmsghdr *nlh, struct nlattr *attrs[], struct dn_fib_info *fi)
> >  {
> >  	struct rtnexthop *nhp;
> > -	int nhlen;
> > +	int nhlen, nhsel;
> >
> >  	if (attrs[RTA_PRIORITY] &&
> >  	    nla_get_u32(attrs[RTA_PRIORITY]) != fi->fib_priority)
> > @@ -246,8 +241,9 @@ static int dn_fib_nh_match(struct rtmsg *r, struct nlmsghdr *nlh, struct nlattr
> >  	nhp = nla_data(attrs[RTA_MULTIPATH]);
> >  	nhlen = nla_len(attrs[RTA_MULTIPATH]);
> >
> > -	for_nexthops(fi) {
> > +	for (nhsel = 0; nhsel < fi->fib_nhs; nhsel++) {
> >  		int attrlen = nhlen - sizeof(struct rtnexthop);
> > +		const struct dn_fib_nh *nh = &fi->fib_nh[nhsel];
> >  		__le16 gw;
> >
> >  		if (attrlen < 0 || (nhlen -= nhp->rtnh_len) < 0)
> > @@ -264,7 +260,7 @@ static int dn_fib_nh_match(struct rtmsg *r, struct nlmsghdr *nlh, struct nlattr
> >  				return 1;
> >  		}
> >  		nhp = RTNH_NEXT(nhp);
> > -	} endfor_nexthops(fi);
> > +	}
> >
> >  	return 0;
> >  }
> > @@ -345,11 +341,13 @@ static int dn_fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
> >  	if (fi->fib_nhs > 1) {
> >  		struct rtnexthop *nhp;
> >  		struct nlattr *mp_head;
> > +		int nhsel;
> >
> >  		if (!(mp_head = nla_nest_start(skb, RTA_MULTIPATH)))
> >  			goto errout;
> >
> > -		for_nexthops(fi) {
> > +		for (nhsel = 0; nhsel < fi->fib_nhs; nhsel++) {
> > +			const struct dn_fib_nh *nh = &fi->fib_nh[nhsel];
> >  			if (!(nhp = nla_reserve_nohdr(skb, sizeof(*nhp))))
> >  				goto errout;
> >
> > @@ -362,7 +360,7 @@ static int dn_fib_dump_info(struct sk_buff *skb, u32 portid, u32 seq, int event,
> >  				goto errout;
> >
> >  			nhp->rtnh_len = skb_tail_pointer(skb) - (unsigned char *)nhp;
> > -		} endfor_nexthops(fi);
> > +		}
> >
> >  		nla_nest_end(skb, mp_head);
> >  	}
> > --
> > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>