| Message ID | 1343979790-17408-1-git-send-email-sakiwit@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jean Sacren <sakiwit@gmail.com> Date: Fri, 3 Aug 2012 01:43:10 -0600 > It is always wrong to write 'return (...)'. In your imagination. > After removal, realign the code where necessary. > > Signed-off-by: Jean Sacren <sakiwit@gmail.com> > - return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & > - (IN4_ADDR_HSIZE - 1)); > + return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & > + (IN4_ADDR_HSIZE - 1); Those parenthesis are there to make the evaluation order and grouping explicit. The other ones you changed are wrong for similar reasons. I absolutely am not applying patches like this. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2012-08-03 at 01:43 -0600, Jean Sacren wrote: > @@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr) > { > u32 val = (__force u32) addr ^ hash_ptr(net, 8); > > - return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & > - (IN4_ADDR_HSIZE - 1)); > + return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & > + (IN4_ADDR_HSIZE - 1); > } BTW This should use a faster implementation, I'll send a patch when net-next is opened. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <eric.dumazet@gmail.com> Date: Fri, 03 Aug 2012 11:19:44 +0200 > On Fri, 2012-08-03 at 01:43 -0600, Jean Sacren wrote: > >> @@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr) >> { >> u32 val = (__force u32) addr ^ hash_ptr(net, 8); >> >> - return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & >> - (IN4_ADDR_HSIZE - 1)); >> + return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & >> + (IN4_ADDR_HSIZE - 1); >> } > > BTW This should use a faster implementation, I'll send a patch when > net-next is opened. There seems to be a few spots where we want the pointer "as a 32-bit integer" for hashing. We were discussing arp_hashfn() and ndisc_hashfn() the other day. It should basically do something like: (u32) ((u64)ptr >> 32 | ((u32) ptr)) on 64-bit and simply (u32)(ptr) on 32-bit. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: David Miller <davem@davemloft.net> Date: Fri, 03 Aug 2012 01:52:43 -0700 > > From: Jean Sacren <sakiwit@gmail.com> > Date: Fri, 3 Aug 2012 01:43:10 -0600 > > > Signed-off-by: Jean Sacren <sakiwit@gmail.com> > > > - return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & > > - (IN4_ADDR_HSIZE - 1)); > > + return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & > > + (IN4_ADDR_HSIZE - 1); > > Those parenthesis are there to make the evaluation order and > grouping explicit. > > The other ones you changed are wrong for similar reasons. Barring the speed issue raised by Eric Dumazet, this patch is correct. To illustrate, the patch merely changes return (A); to return A; where A is nothing but an expression, regardless if it is a simple one or a compound one. Whatever A is, it evaluates to a value. The parentheses do _not_ contribute to the evaluation. They do not alter precedence. They do not make the evaluation order explicit. They are there to serve no purpose, yet they do make return statement look like a function call. Moreover, A is one single entity. It does not group with anything else. I really don't see the significance of making A a group by itself. Thank you for reviewing the patch. With all due respect, your assessment is hard to agree with this moment.
From: Jean Sacren <sakiwit@gmail.com> Date: Fri, 3 Aug 2012 14:23:50 -0600 > To illustrate, the patch merely changes > > return (A); > > to > > return A; Life is not black and white my friend, there are shades of gray. I feel bad for you if you read coding style rules in a %100 literal sense with no room for interpretation. People add extra parenthesis to add clarity, so you are entirely misrepresenting these cases. This was not a simple case of: return (A); but more like something that looks as: return ((A & B) | (C ^ D)); and that is perfectly fine. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index 44bf82e..72b16e4 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -106,8 +106,8 @@ static inline unsigned int inet_addr_hash(struct net *net, __be32 addr) { u32 val = (__force u32) addr ^ hash_ptr(net, 8); - return ((val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & - (IN4_ADDR_HSIZE - 1)); + return (val ^ (val >> 8) ^ (val >> 16) ^ (val >> 24)) & + (IN4_ADDR_HSIZE - 1); } static void inet_hash_insert(struct net *net, struct in_ifaddr *ifa) diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c index e1e0a4e..8049ce0 100644 --- a/net/ipv4/inetpeer.c +++ b/net/ipv4/inetpeer.c @@ -96,7 +96,7 @@ static atomic_t v6_seq = ATOMIC_INIT(0); static atomic_t *inetpeer_seq_ptr(int family) { - return (family == AF_INET ? &v4_seq : &v6_seq); + return (family == AF_INET) ? &v4_seq : &v6_seq; } static inline void flush_check(struct inet_peer_base *base, int family) diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c index 650e152..cb9c489 100644 --- a/net/ipv4/syncookies.c +++ b/net/ipv4/syncookies.c @@ -103,10 +103,10 @@ static __u32 secure_tcp_syn_cookie(__be32 saddr, __be32 daddr, __be16 sport, * MSS into the second hash value. */ - return (cookie_hash(saddr, daddr, sport, dport, 0, 0) + - sseq + (count << COOKIEBITS) + - ((cookie_hash(saddr, daddr, sport, dport, count, 1) + data) - & COOKIEMASK)); + return cookie_hash(saddr, daddr, sport, dport, 0, 0) + + sseq + (count << COOKIEBITS) + + ((cookie_hash(saddr, daddr, sport, dport, count, 1) + data) + & COOKIEMASK); } /* diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 2fd2bc9..1d220f9 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3993,17 +3993,17 @@ static int tcp_disordered_ack(const struct sock *sk, const struct sk_buff *skb) u32 seq = TCP_SKB_CB(skb)->seq; u32 ack = TCP_SKB_CB(skb)->ack_seq; - return (/* 1. Pure ACK with correct sequence number. */ - (th->ack && seq == TCP_SKB_CB(skb)->end_seq && seq == tp->rcv_nxt) && + return /* 1. Pure ACK with correct sequence number. */ + (th->ack && seq == TCP_SKB_CB(skb)->end_seq && seq == tp->rcv_nxt) && - /* 2. ... and duplicate ACK. */ - ack == tp->snd_una && + /* 2. ... and duplicate ACK. */ + ack == tp->snd_una && - /* 3. ... and does not update window. */ - !tcp_may_update_window(tp, ack, seq, ntohs(th->window) << tp->rx_opt.snd_wscale) && + /* 3. ... and does not update window. */ + !tcp_may_update_window(tp, ack, seq, ntohs(th->window) << tp->rx_opt.snd_wscale) && - /* 4. ... and sits in replay window. */ - (s32)(tp->rx_opt.ts_recent - tp->rx_opt.rcv_tsval) <= (inet_csk(sk)->icsk_rto * 1024) / HZ); + /* 4. ... and sits in replay window. */ + (s32)(tp->rx_opt.ts_recent - tp->rx_opt.rcv_tsval) <= (inet_csk(sk)->icsk_rto * 1024) / HZ; } static inline bool tcp_paws_discard(const struct sock *sk, diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index a7b3ec9..eac214c 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -1557,7 +1557,7 @@ static inline unsigned int tcp_cwnd_test(const struct tcp_sock *tp, in_flight = tcp_packets_in_flight(tp); cwnd = tp->snd_cwnd; if (in_flight < cwnd) - return (cwnd - in_flight); + return cwnd - in_flight; return 0; }
It is always wrong to write 'return (...)'. After removal, realign the code where necessary. Signed-off-by: Jean Sacren <sakiwit@gmail.com> --- net/ipv4/devinet.c | 4 ++-- net/ipv4/inetpeer.c | 2 +- net/ipv4/syncookies.c | 8 ++++---- net/ipv4/tcp_input.c | 16 ++++++++-------- net/ipv4/tcp_output.c | 2 +- 5 files changed, 16 insertions(+), 16 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html