| Message ID | 20230706134226.9119-1-machel@vivo.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v1] net:tipc:Remove repeated initialization | expand |
On Thu, 6 Jul 2023 21:42:09 +0800 Wang Ming wrote: > The original code initializes 'tmp' twice, > which causes duplicate initialization issue. > To fix this, we remove the second initialization > of 'tmp' and use 'parent' directly forsubsequent > operations. > > Signed-off-by: Wang Ming <machel@vivo.com> Please stop sending the "remove repeated initialization" patches to networking, thanks.
Le 06/07/2023 à 17:47, Jakub Kicinski a écrit : > On Thu, 6 Jul 2023 21:42:09 +0800 Wang Ming wrote: >> The original code initializes 'tmp' twice, >> which causes duplicate initialization issue. >> To fix this, we remove the second initialization >> of 'tmp' and use 'parent' directly forsubsequent >> operations. >> >> Signed-off-by: Wang Ming <machel@vivo.com> > > Please stop sending the "remove repeated initialization" patches > to networking, thanks. > > The patch also looks just bogus, as 'parent' is now always NULL when: rb_link_node(&m->tree_node, parent, n); is called after the while loop. CJ
> The original code initializes 'tmp' twice, > which causes duplicate initialization issue. Is it more appropriate to refer to a repetition of questionable variable assignments? > To fix this, we remove the second initialization > of 'tmp' and use 'parent' directly forsubsequent > operations. * Would you like to avoid a typo in this sentence? * Please choose a better imperative change suggestion. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94 … > +++ b/net/tipc/group.c > @@ -284,8 +284,6 @@ static int tipc_group_add_to_tree(struct tipc_group *grp, > n = &grp->members.rb_node; > while (*n) { > tmp = container_of(*n, struct tipc_member, tree_node); > - parent = *n; > - tmp = container_of(parent, struct tipc_member, tree_node); > nkey = (u64)tmp->node << 32 | tmp->port; … How does the proposed deletion fit to the function call “rb_link_node(&m->tree_node, parent, n)” after the loop? https://elixir.bootlin.com/linux/v6.4.2/source/net/tipc/group.c#L277 Regards, Markus
Hi Jakub Kicinski : ) I understand, but I am confused about whether my modification is wrong? -----邮件原件----- 发件人: Jakub Kicinski <kuba@kernel.org> 发送时间: 2023年7月6日 23:47 收件人: 王明-软件底层技术部 <machel@vivo.com> 抄送: Jon Maloy <jmaloy@redhat.com>; Ying Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com> 主题: Re: [PATCH v1] net:tipc:Remove repeated initialization [Some people who received this message don't often get email from kuba@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] On Thu, 6 Jul 2023 21:42:09 +0800 Wang Ming wrote: > The original code initializes 'tmp' twice, which causes duplicate > initialization issue. > To fix this, we remove the second initialization of 'tmp' and use > 'parent' directly forsubsequent operations. > > Signed-off-by: Wang Ming <machel@vivo.com> Please stop sending the "remove repeated initialization" patches to networking, thanks.
Hi CJ : ) So what you're saying is there's no repeat initialization problem here. -----邮件原件----- 发件人: Christophe JAILLET <christophe.jaillet@wanadoo.fr> 发送时间: 2023年7月7日 1:14 收件人: 王明-软件底层技术部 <machel@vivo.com> 抄送: Jon Maloy <jmaloy@redhat.com>; Ying Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>; Jakub Kicinski <kuba@kernel.org> 主题: Re: [PATCH v1] net:tipc:Remove repeated initialization [你通常不会收到来自 christophe.jaillet@wanadoo.fr 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要] Le 06/07/2023 à 17:47, Jakub Kicinski a écrit : > On Thu, 6 Jul 2023 21:42:09 +0800 Wang Ming wrote: >> The original code initializes 'tmp' twice, which causes duplicate >> initialization issue. >> To fix this, we remove the second initialization of 'tmp' and use >> 'parent' directly forsubsequent operations. >> >> Signed-off-by: Wang Ming <machel@vivo.com> > > Please stop sending the "remove repeated initialization" patches to > networking, thanks. > > The patch also looks just bogus, as 'parent' is now always NULL when: rb_link_node(&m->tree_node, parent, n); is called after the while loop. CJ
On Fri, 7 Jul 2023 01:21:52 +0000 王明-软件底层技术部 wrote: > Hi Jakub Kicinski > : ) > I understand, but I am confused about whether my modification is wrong? The changes are so trivial they are not worth spending time on. And you haven't read: https://www.kernel.org/doc/html/next/process/index.html
Le 07/07/2023 à 03:24, 王明-软件底层技术部 a écrit : > Hi CJ > : ) > So what you're saying is there's no repeat initialization problem here. Hi, First, top posting is usually not the better way to answer email. Anyway, in your patch, you have: n = &grp->members.rb_node; while (*n) { tmp = container_of(*n, struct tipc_member, tree_node); - parent = *n; - tmp = container_of(parent, struct tipc_member, tree_node); nkey = (u64)tmp->node << 32 | tmp->port; if (key < nkey) n = &(*n)->rb_left; You are right, 'tmp' is initialized twice. It is even initialized twice, with the same walue. But in your patch, you also remove "parent = *n;" So now, 'parent' is never updated in the function, and when rb_link_node() is called after the while loop, it is NULL. So your patch changed the behaviour of the code and looks broken. Something like: while (*n) { tmp = container_of(*n, struct tipc_member, tree_node); parent = *n; - tmp = container_of(parent, struct tipc_member, tree_node); nkey = (u64)tmp->node << 32 | tmp->port; if (key < nkey) n = &(*n)->rb_left; would look good to me but, as said by Jakub in the thread, is really unlikely to be applied. The risk of breaking code (as you un-intentionally did) is higher than the value of removing a redundant initialization. Reviewing such patches also consume maintainers' time to check for such potential errors and sometimes it is safer to leave things as-is in order not to waste time and avoid potential troubles. CJ > > -----邮件原件----- > 发件人: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > 发送时间: 2023年7月7日 1:14 > 收件人: 王明-软件底层技术部 <machel@vivo.com> > 抄送: Jon Maloy <jmaloy@redhat.com>; Ying Xue <ying.xue@windriver.com>; David S. Miller <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; netdev@vger.kernel.org; tipc-discussion@lists.sourceforge.net; linux-kernel@vger.kernel.org; opensource.kernel <opensource.kernel@vivo.com>; Jakub Kicinski <kuba@kernel.org> > 主题: Re: [PATCH v1] net:tipc:Remove repeated initialization > > [你通常不会收到来自 christophe.jaillet@wanadoo.fr 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解这一点为什么很重要] > > Le 06/07/2023 à 17:47, Jakub Kicinski a écrit : >> On Thu, 6 Jul 2023 21:42:09 +0800 Wang Ming wrote: >>> The original code initializes 'tmp' twice, which causes duplicate >>> initialization issue. >>> To fix this, we remove the second initialization of 'tmp' and use >>> 'parent' directly forsubsequent operations. >>> >>> Signed-off-by: Wang Ming <machel@vivo.com> >> >> Please stop sending the "remove repeated initialization" patches to >> networking, thanks. >> >> > > The patch also looks just bogus, as 'parent' is now always NULL when: > rb_link_node(&m->tree_node, parent, n); > > is called after the while loop. > > CJ
> The risk of breaking code (as you un-intentionally did) is higher than > the value of removing a redundant initialization. I find that it would be nicer to perform only required data processing steps. The application of variable assignments can occasionally be improved further. The corresponding efforts grow for proper code review. The discussed change possibilities might belong to the adjustment category “code cleanup”. Some contributors have got difficulties to integrate presented ideas. Thus it seems that more attractive incentives need to be offered for potentially desirable software updates. Regards, Markus
> - parent = *n; > - tmp = container_of(parent, struct tipc_member, tree_node); > nkey = (u64)tmp->node << 32 | tmp->port; > if (key < nkey) > n = &(*n)->rb_left; [RESEND] Your patch breaks my test suite. You introduced a new bug which was pointed out by Christophe. It is a redundant assignment to variable tmp as you pointed out. I suggest that you repost this cleanup together with other patches that you see need to be refactored (I believe there are still many things like this in TIPC) in the future, or I can do what you suggested in my future cleanup.
diff --git a/net/tipc/group.c b/net/tipc/group.c index 3e137d8c9d2f..b2f964f62c36 100644 --- a/net/tipc/group.c +++ b/net/tipc/group.c @@ -284,8 +284,6 @@ static int tipc_group_add_to_tree(struct tipc_group *grp, n = &grp->members.rb_node; while (*n) { tmp = container_of(*n, struct tipc_member, tree_node); - parent = *n; - tmp = container_of(parent, struct tipc_member, tree_node); nkey = (u64)tmp->node << 32 | tmp->port; if (key < nkey) n = &(*n)->rb_left;
The original code initializes 'tmp' twice, which causes duplicate initialization issue. To fix this, we remove the second initialization of 'tmp' and use 'parent' directly forsubsequent operations. Signed-off-by: Wang Ming <machel@vivo.com> --- net/tipc/group.c | 2 -- 1 file changed, 2 deletions(-)