Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1263074 > unrolled thread

[PATCH] decnet: remove macro-local declarations

Back to article view | Back to linux.kernel

Contents

  [PATCH] decnet: remove macro-local declarations Julia Lawall <Julia.Lawall@lip6.fr> - 2015-11-05 11:30 +0100
    Re: [PATCH] decnet: remove macro-local declarations David Miller <davem@davemloft.net> - 2015-11-05 20:30 +0100
      Re: [PATCH] decnet: remove macro-local declarations Julia Lawall <julia.lawall@lip6.fr> - 2015-11-05 20:40 +0100
        Re: [PATCH] decnet: remove macro-local declarations Joe Perches <joe@perches.com> - 2015-11-05 23:10 +0100
          Re: [PATCH] decnet: remove macro-local declarations Julia Lawall <julia.lawall@lip6.fr> - 2015-11-05 23:30 +0100
      Re: [PATCH] decnet: remove macro-local declarations Julia Lawall <julia.lawall@lip6.fr> - 2015-11-05 21:10 +0100
        Re: [PATCH] decnet: remove macro-local declarations David Miller <davem@davemloft.net> - 2015-11-05 21:20 +0100
          Re: [PATCH] decnet: remove macro-local declarations Julia Lawall <julia.lawall@lip6.fr> - 2015-11-06 12:00 +0100
            Re: [PATCH] decnet: remove macro-local declarations walter harms <wharms@bfs.de> - 2015-11-06 12:50 +0100
              Re: [PATCH] decnet: remove macro-local declarations Julia Lawall <julia.lawall@lip6.fr> - 2015-11-06 12:50 +0100
            Re: [PATCH] decnet: remove macro-local declarations David Miller <davem@davemloft.net> - 2015-11-07 19:20 +0100
              Re: [PATCH] decnet: remove macro-local declarations Julia Lawall <julia.lawall@lip6.fr> - 2015-11-07 19:30 +0100

#1263074 — [PATCH] decnet: remove macro-local declarations

Move the variable declarations from the for_nexthops macro to the
surrounding context, so that it is clear where these variables are
declared.  This also makes it possible to remove the endfor_nexthops macro.

This change adds new arguments to the macro for_nexthops.  They are ordered
such that a pointer to the referenced object comes first, the index in the
list comes next, and the list itself comes last, roughly in analogy with
the list_for_each macros.

Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>

---

This patch takes care of a single file, where the macros are defined
locally.  If the basic transformation looks OK, I will change the other
files that either likewise define their own macros or use the macros in
net/mpls/internal.h.  The potentially affected files are:

net/decnet/dn_fib.c
net/ipv4/fib_semantics.c
net/mpls/af_mpls.c

 net/decnet/dn_table.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/decnet/dn_table.c b/net/decnet/dn_table.c
index 1540b50..3e20dbb 100644
--- a/net/decnet/dn_table.c
+++ b/net/decnet/dn_table.c
@@ -60,11 +60,9 @@ struct dn_hash
 
 #define dz_key_0(key)		((key).datum = 0)
 
-#define for_nexthops(fi) { int nhsel; const struct dn_fib_nh *nh;\
+#define for_nexthops(nh, nhsel, fi) \
 	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
@@ -226,8 +224,9 @@ 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)
 {
+	const struct dn_fib_nh *nh;
 	struct rtnexthop *nhp;
-	int nhlen;
+	int nhlen, nhsel;
 
 	if (attrs[RTA_PRIORITY] &&
 	    nla_get_u32(attrs[RTA_PRIORITY]) != fi->fib_priority)
@@ -246,7 +245,7 @@ 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_nexthops(nh, nhsel, fi) {
 		int attrlen = nhlen - sizeof(struct rtnexthop);
 		__le16 gw;
 
@@ -264,7 +263,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 +344,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;
+		const struct dn_fib_nh *nh;
+		int nhsel;
 
 		if (!(mp_head = nla_nest_start(skb, RTA_MULTIPATH)))
 			goto errout;
 
-		for_nexthops(fi) {
+		for_nexthops(nh, nhsel, fi) {
 			if (!(nhp = nla_reserve_nohdr(skb, sizeof(*nhp))))
 				goto errout;
 
@@ -362,7 +363,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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1263504

From: Julia Lawall <Julia.Lawall@lip6.fr>
Date: Thu,  5 Nov 2015 11:18:16 +0100

> Move the variable declarations from the for_nexthops macro to the
> surrounding context, so that it is clear where these variables are
> declared.  This also makes it possible to remove the endfor_nexthops macro.
> 
> This change adds new arguments to the macro for_nexthops.  They are ordered
> such that a pointer to the referenced object comes first, the index in the
> list comes next, and the list itself comes last, roughly in analogy with
> the list_for_each macros.
> 
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> 
> ---
> 
> This patch takes care of a single file, where the macros are defined
> locally.  If the basic transformation looks OK, I will change the other
> files that either likewise define their own macros or use the macros in
> net/mpls/internal.h.  The potentially affected files are:
 ...

This looks fine to me.

Please resubmit this when net-next opens back up, which should be
shortly after -rc1.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263510

On Thu, 5 Nov 2015, David Miller wrote:

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> Date: Thu,  5 Nov 2015 11:18:16 +0100
> 
> > Move the variable declarations from the for_nexthops macro to the
> > surrounding context, so that it is clear where these variables are
> > declared.  This also makes it possible to remove the endfor_nexthops macro.
> > 
> > This change adds new arguments to the macro for_nexthops.  They are ordered
> > such that a pointer to the referenced object comes first, the index in the
> > list comes next, and the list itself comes last, roughly in analogy with
> > the list_for_each macros.
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> > 
> > This patch takes care of a single file, where the macros are defined
> > locally.  If the basic transformation looks OK, I will change the other
> > files that either likewise define their own macros or use the macros in
> > net/mpls/internal.h.  The potentially affected files are:
>  ...
> 
> This looks fine to me.
> 
> Please resubmit this when net-next opens back up, which should be
> shortly after -rc1.

OK, I'll do the others then too.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263573

On Thu, 2015-11-05 at 20:38 +0100, Julia Lawall wrote:
> On Thu, 5 Nov 2015, David Miller wrote:
> > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > Date: Thu,  5 Nov 2015 11:18:16 +0100> 
> > > Move the variable declarations from the for_nexthops macro to the
> > > surrounding context, so that it is clear where these variables are
> > > declared.  This also makes it possible to remove the endfor_nexthops macro.
> > > 
> > > This change adds new arguments to the macro for_nexthops.  They are ordered
> > > such that a pointer to the referenced object comes first, the index in the
> > > list comes next, and the list itself comes last, roughly in analogy with
> > > the list_for_each macros.
[]
> > > This patch takes care of a single file, where the macros are defined
> > > locally.  If the basic transformation looks OK, I will change the other
> > > files that either likewise define their own macros or use the macros in
> > > net/mpls/internal.h.  The potentially affected files are:
> >  ...
> > 
> > This looks fine to me.
> > 
> > Please resubmit this when net-next opens back up, which should be
> > shortly after -rc1.
> 
> OK, I'll do the others then too.

If you do can you please parenthesize the macro arguments?

#define for_nexthops(nh, nhsel, fi) \
 	for (nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; (nh)++, (nhsel)++)
instead of
	for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)

And perhaps a renaming might be better

	s/for_nexthops/for_each_nexthop/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263586

On Thu, 5 Nov 2015, Joe Perches wrote:

> On Thu, 2015-11-05 at 20:38 +0100, Julia Lawall wrote:
> > On Thu, 5 Nov 2015, David Miller wrote:
> > > From: Julia Lawall <Julia.Lawall@lip6.fr>
> > > Date: Thu,  5 Nov 2015 11:18:16 +0100> 
> > > > Move the variable declarations from the for_nexthops macro to the
> > > > surrounding context, so that it is clear where these variables are
> > > > declared.  This also makes it possible to remove the endfor_nexthops macro.
> > > > 
> > > > This change adds new arguments to the macro for_nexthops.  They are ordered
> > > > such that a pointer to the referenced object comes first, the index in the
> > > > list comes next, and the list itself comes last, roughly in analogy with
> > > > the list_for_each macros.
> []
> > > > This patch takes care of a single file, where the macros are defined
> > > > locally.  If the basic transformation looks OK, I will change the other
> > > > files that either likewise define their own macros or use the macros in
> > > > net/mpls/internal.h.  The potentially affected files are:
> > >  ...
> > > 
> > > This looks fine to me.
> > > 
> > > Please resubmit this when net-next opens back up, which should be
> > > shortly after -rc1.
> > 
> > OK, I'll do the others then too.
> 
> If you do can you please parenthesize the macro arguments?
> 
> #define for_nexthops(nh, nhsel, fi) \
>  	for (nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; (nh)++, (nhsel)++)
> instead of
> 	for(nhsel = 0, nh = (fi)->fib_nh; nhsel < (fi)->fib_nhs; nh++, nhsel++)

OK

> And perhaps a renaming might be better
> 
> 	s/for_nexthops/for_each_nexthop/

OK

Thanks for the suggestions.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263527

On Thu, 5 Nov 2015, David Miller wrote:

> From: Julia Lawall <Julia.Lawall@lip6.fr>
> Date: Thu,  5 Nov 2015 11:18:16 +0100
> 
> > Move the variable declarations from the for_nexthops macro to the
> > surrounding context, so that it is clear where these variables are
> > declared.  This also makes it possible to remove the endfor_nexthops macro.
> > 
> > This change adds new arguments to the macro for_nexthops.  They are ordered
> > such that a pointer to the referenced object comes first, the index in the
> > list comes next, and the list itself comes last, roughly in analogy with
> > the list_for_each macros.
> > 
> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
> > 
> > ---
> > 
> > This patch takes care of a single file, where the macros are defined
> > locally.  If the basic transformation looks OK, I will change the other
> > files that either likewise define their own macros or use the macros in
> > net/mpls/internal.h.  The potentially affected files are:
>  ...
> 
> This looks fine to me.

Would it be preferable to remove the macro entirely and inline the for 
loop header?

julia

> Please resubmit this when net-next opens back up, which should be
> shortly after -rc1.
> 
> Thanks.
> --
> 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263535

From: Julia Lawall <julia.lawall@lip6.fr>
Date: Thu, 5 Nov 2015 21:08:17 +0100 (CET)

> On Thu, 5 Nov 2015, David Miller wrote:
> 
>> From: Julia Lawall <Julia.Lawall@lip6.fr>
>> Date: Thu,  5 Nov 2015 11:18:16 +0100
>> 
>> > Move the variable declarations from the for_nexthops macro to the
>> > surrounding context, so that it is clear where these variables are
>> > declared.  This also makes it possible to remove the endfor_nexthops macro.
>> > 
>> > This change adds new arguments to the macro for_nexthops.  They are ordered
>> > such that a pointer to the referenced object comes first, the index in the
>> > list comes next, and the list itself comes last, roughly in analogy with
>> > the list_for_each macros.
>> > 
>> > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
>> > 
>> > ---
>> > 
>> > This patch takes care of a single file, where the macros are defined
>> > locally.  If the basic transformation looks OK, I will change the other
>> > files that either likewise define their own macros or use the macros in
>> > net/mpls/internal.h.  The potentially affected files are:
>>  ...
>> 
>> This looks fine to me.
> 
> 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?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263917

> > 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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263939

+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.

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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1263943

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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264907

From: Julia Lawall <julia.lawall@lip6.fr>
Date: Fri, 6 Nov 2015 11:57:34 +0100 (CET)

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

Personally I like the explicit named iterator.  It is descriptive and
tells that we are walking over all of the nexthops for a route.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1264912

On Sat, 7 Nov 2015, David Miller wrote:

> From: Julia Lawall <julia.lawall@lip6.fr>
> Date: Fri, 6 Nov 2015 11:57:34 +0100 (CET)
> 
> >> > 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.
> 
> Personally I like the explicit named iterator.  It is descriptive and
> tells that we are walking over all of the nexthops for a route.

OK, I'll follow that solution.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web