| Message ID | 20230710100624.87836-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,resend,v1,1/1] netlink: Don't use int as bool in netlink_update_socket_mc() | expand |
On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > The bit operations take boolean parameter and return also boolean > (in test_bit()-like cases). Don't threat booleans as integers when > it's not needed. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > net/netlink/af_netlink.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > index 383631873748..d81e7a43944c 100644 > --- a/net/netlink/af_netlink.c > +++ b/net/netlink/af_netlink.c > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > /* must be called with netlink table grabbed */ > static void netlink_update_socket_mc(struct netlink_sock *nlk, > unsigned int group, > - int is_new) > + bool new) > { > - int old, new = !!is_new, subscriptions; > + int subscriptions; > + bool old; > > old = test_bit(group - 1, nlk->groups); > subscriptions = nlk->subscriptions - old + new; So what is the outcome of "int - bool + bool" in the line above? > @@ -2152,7 +2153,7 @@ void __netlink_clear_multicast_users(struct sock *ksk, unsigned int group) > struct netlink_table *tbl = &nl_table[ksk->sk_protocol]; > > sk_for_each_bound(sk, &tbl->mc_list) > - netlink_update_socket_mc(nlk_sk(sk), group, 0); > + netlink_update_socket_mc(nlk_sk(sk), group, false); > } > > struct nlmsghdr * > -- > 2.40.0.1.gaa8946217a0b > >
On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > The bit operations take boolean parameter and return also boolean > > (in test_bit()-like cases). Don't threat booleans as integers when > > it's not needed. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > --- > > net/netlink/af_netlink.c | 7 ++++--- > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > index 383631873748..d81e7a43944c 100644 > > --- a/net/netlink/af_netlink.c > > +++ b/net/netlink/af_netlink.c > > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > > /* must be called with netlink table grabbed */ > > static void netlink_update_socket_mc(struct netlink_sock *nlk, > > unsigned int group, > > - int is_new) > > + bool new) > > { > > - int old, new = !!is_new, subscriptions; > > + int subscriptions; > > + bool old; > > > > old = test_bit(group - 1, nlk->groups); > > subscriptions = nlk->subscriptions - old + new; > > So what is the outcome of "int - bool + bool" in the line above? FTR, I agree with Leon, the old code is more readable to me/I don't see a practical gain with this change. Cheers, Paolo
On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > > The bit operations take boolean parameter and return also boolean > > > (in test_bit()-like cases). Don't threat booleans as integers when > > > it's not needed. > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > --- > > > net/netlink/af_netlink.c | 7 ++++--- > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > > index 383631873748..d81e7a43944c 100644 > > > --- a/net/netlink/af_netlink.c > > > +++ b/net/netlink/af_netlink.c > > > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > > > /* must be called with netlink table grabbed */ > > > static void netlink_update_socket_mc(struct netlink_sock *nlk, > > > unsigned int group, > > > - int is_new) > > > + bool new) > > > { > > > - int old, new = !!is_new, subscriptions; > > > + int subscriptions; > > > + bool old; > > > > > > old = test_bit(group - 1, nlk->groups); > > > subscriptions = nlk->subscriptions - old + new; > > > > So what is the outcome of "int - bool + bool" in the line above? The same as with int - int [0 .. 1] + int [0 .. 1]. Note, the code _already_ uses boolean as integers. > FTR, I agree with Leon, the old code is more readable to me/I don't see > a practical gain with this change. This change does not change the status quo. The code uses booleans as integers already (in the callers). As I mentioned earlier, the purity of the code (converting booleans to integers beforehand) adds unneeded churn and with this change code becomes cleaner at least for the (existing) callers.
On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > > > The bit operations take boolean parameter and return also boolean > > > > (in test_bit()-like cases). Don't threat booleans as integers when > > > > it's not needed. > > > > > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > > > --- > > > > net/netlink/af_netlink.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c > > > > index 383631873748..d81e7a43944c 100644 > > > > --- a/net/netlink/af_netlink.c > > > > +++ b/net/netlink/af_netlink.c > > > > @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); > > > > /* must be called with netlink table grabbed */ > > > > static void netlink_update_socket_mc(struct netlink_sock *nlk, > > > > unsigned int group, > > > > - int is_new) > > > > + bool new) > > > > { > > > > - int old, new = !!is_new, subscriptions; > > > > + int subscriptions; > > > > + bool old; > > > > > > > > old = test_bit(group - 1, nlk->groups); > > > > subscriptions = nlk->subscriptions - old + new; > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > The same as with int - int [0 .. 1] + int [0 .. 1]. No, it is not. bool is defined as _Bool C99 type, so strictly speaking you are mixing types int - _Bool + _Bool. Thanks > > Note, the code _already_ uses boolean as integers. > > > FTR, I agree with Leon, the old code is more readable to me/I don't see > > a practical gain with this change. > > This change does not change the status quo. The code uses booleans as integers > already (in the callers). > > As I mentioned earlier, the purity of the code (converting booleans to integers > beforehand) adds unneeded churn and with this change code becomes cleaner at > least for the (existing) callers. > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: ... > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > you are mixing types int - _Bool + _Bool. 1. The original code already does that. You still haven't reacted on that. 2. Is what you are telling a problem?
On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > ... > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > you are mixing types int - _Bool + _Bool. > > 1. The original code already does that. You still haven't reacted on that. The original code was int - int + int. > 2. Is what you are telling a problema? No, I'm saying that you took perfectly correct code which had all types aligned and changed it to have mixed type arithmetic. Thanks > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Jul 11, 2023 at 04:32:59PM +0300, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: ... > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > > you are mixing types int - _Bool + _Bool. > > > > 1. The original code already does that. You still haven't reacted on that. > > The original code was int - int + int. No. You missed the callers part. They are using boolean. > > 2. Is what you are telling a problema? > > No, I'm saying that you took perfectly correct code which had all types > aligned and changed it to have mixed type arithmetic. And after this change it's perfectly correct code with less letters and hidden promotions (as a parameter to the function) and hence requires less cognitive energy to parse. So, the bottom line is the commit message you don't like, is it so?
On Tue, Jul 11, 2023 at 04:44:18PM +0300, Andy Shevchenko wrote: > On Tue, Jul 11, 2023 at 04:32:59PM +0300, Leon Romanovsky wrote: > > On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > > > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > > > you are mixing types int - _Bool + _Bool. > > > > > > 1. The original code already does that. You still haven't reacted on that. > > > > The original code was int - int + int. > > No. You missed the callers part. They are using boolean. I didn't miss and pointed you to the exact line which was implicitly changed with your patch. > > > > 2. Is what you are telling a problema? > > > > No, I'm saying that you took perfectly correct code which had all types > > aligned and changed it to have mixed type arithmetic. > > And after this change it's perfectly correct code with less letters and hidden > promotions (as a parameter to the function) and hence requires less cognitive > energy to parse. > > So, the bottom line is the commit message you don't like, is it so? Please reread my and Paolo replies. Thanks > > -- > With Best Regards, > Andy Shevchenko > >
On Tue, Jul 11, 2023 at 08:10:58PM +0300, Leon Romanovsky wrote: > On Tue, Jul 11, 2023 at 04:44:18PM +0300, Andy Shevchenko wrote: > > On Tue, Jul 11, 2023 at 04:32:59PM +0300, Leon Romanovsky wrote: > > > On Tue, Jul 11, 2023 at 03:45:34PM +0300, Andy Shevchenko wrote: > > > > On Tue, Jul 11, 2023 at 03:20:12PM +0300, Leon Romanovsky wrote: > > > > > On Tue, Jul 11, 2023 at 01:54:18PM +0300, Andy Shevchenko wrote: > > > > > > On Tue, Jul 11, 2023 at 12:21:12PM +0200, Paolo Abeni wrote: > > > > > > > On Tue, 2023-07-11 at 09:33 +0300, Leon Romanovsky wrote: > > > > > > > > On Mon, Jul 10, 2023 at 01:06:24PM +0300, Andy Shevchenko wrote: ... > > > > > > > > So what is the outcome of "int - bool + bool" in the line above? > > > > > > > > > > > > The same as with int - int [0 .. 1] + int [0 .. 1]. > > > > > > > > > > No, it is not. bool is defined as _Bool C99 type, so strictly speaking > > > > > you are mixing types int - _Bool + _Bool. > > > > > > > > 1. The original code already does that. You still haven't reacted on that. > > > > > > The original code was int - int + int. > > > > No. You missed the callers part. They are using boolean. > > I didn't miss and pointed you to the exact line which was implicitly > changed with your patch. Yes, and this line doesn't change the status quo. We have boolean in the callers that implicitly went to the callee as int. > > > > 2. Is what you are telling a problema? > > > > > > No, I'm saying that you took perfectly correct code which had all types > > > aligned and changed it to have mixed type arithmetic. > > > > And after this change it's perfectly correct code with less letters and hidden > > promotions (as a parameter to the function) and hence requires less cognitive > > energy to parse. > > > > So, the bottom line is the commit message you don't like, is it so? > > Please reread my and Paolo replies. I have read them. My point is that you should also look at the callers to see the big picture.
On Mon, 10 Jul 2023 13:06:24 +0300 Andy Shevchenko wrote: > The bit operations take boolean parameter and return also boolean > (in test_bit()-like cases). Don't threat booleans as integers when > it's not needed. I don't have a strong opinion on the merit. But I feel like the discussion is a waste of everyone's time, so to discourage such ambivalent patches I'm going to drop this.
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index 383631873748..d81e7a43944c 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1623,9 +1623,10 @@ EXPORT_SYMBOL(netlink_set_err); /* must be called with netlink table grabbed */ static void netlink_update_socket_mc(struct netlink_sock *nlk, unsigned int group, - int is_new) + bool new) { - int old, new = !!is_new, subscriptions; + int subscriptions; + bool old; old = test_bit(group - 1, nlk->groups); subscriptions = nlk->subscriptions - old + new; @@ -2152,7 +2153,7 @@ void __netlink_clear_multicast_users(struct sock *ksk, unsigned int group) struct netlink_table *tbl = &nl_table[ksk->sk_protocol]; sk_for_each_bound(sk, &tbl->mc_list) - netlink_update_socket_mc(nlk_sk(sk), group, 0); + netlink_update_socket_mc(nlk_sk(sk), group, false); } struct nlmsghdr *
The bit operations take boolean parameter and return also boolean (in test_bit()-like cases). Don't threat booleans as integers when it's not needed. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- net/netlink/af_netlink.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)