From mboxrd@z Thu Jan 1 00:00:00 1970 From: johan@kernel.org (Johan Hovold) Date: Thu, 1 Oct 2015 13:47:18 -0400 Subject: [Cocci] [PATCH] coccinelle: misc: remove "complex return code" warnings In-Reply-To: References: <1443652647-23097-1-git-send-email-johan@kernel.org> Message-ID: <20151001174718.GJ4284@localhost> To: cocci@systeme.lip6.fr List-Id: cocci@systeme.lip6.fr On Thu, Oct 01, 2015 at 07:20:10AM +0200, Julia Lawall wrote: > On Wed, 30 Sep 2015, Johan Hovold wrote: > > > This effectively reverts 932058a5d5f9 ("coccinelle: misc: semantic patch > > to delete overly complex return code processing"). > > > > There can be both symmetry and readability reasons for not wanting to do > > the final function call as part of the return statement and to maintain > > a clear separation of success and error paths. > > > > Since this is in no way mandated by the coding standard, let's just > > remove this semantic patch to avoid having "clean up" patches being > > posted over and over in response to these Coccinelle warnings. > > What do you mean by "posted"? Are you referring to 0-day build testing > or individual usage of make coccicheck? Maybe it would make sense to > remove the semantic patch from 0-day build testing but leave it in the > kernel, perhaps removing the < 0 case because that one in practice doesn't > seem to turn up much that is useful? Individuals running coccicheck on in-kernel code and posting patches to "fix warnings", where the end result is not necessarily an improvement. But I don't think these warnings should be enabled for 0-day build testing either as it is should be up to the author to decide what style to prefer in each case. > Perhaps it could also be improved to detect a previous != 0 case and then > not return a warning. On some functions, this change can make some nice > simplifications. Yes, that would at least improve things. I don't think warnings should be generated at all for the following code: { int ret; ret = init_a(...); if (ret) return ret; ret = init_b(...); if (ret) return ret; return 0; } as it is (at least to me) preferred over: { int ret; ret = init_a(...); if (ret) return ret; return init_b(...); } for symmetry and readability reasons (e.g. I don't have to look at init_b to figure out what the functions returns). And with a long parameter list to init_b with line breaks, this would look even worse. But either way, it should be up to the author of the code to decide what style to use. Thanks, Johan From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756536AbbJARqA (ORCPT ); Thu, 1 Oct 2015 13:46:00 -0400 Received: from mail-yk0-f181.google.com ([209.85.160.181]:36693 "EHLO mail-yk0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756088AbbJARp6 (ORCPT ); Thu, 1 Oct 2015 13:45:58 -0400 X-Google-Original-Sender: Date: Thu, 1 Oct 2015 13:47:18 -0400 From: Johan Hovold To: Julia Lawall Cc: Johan Hovold , Michal Marek , Gilles Muller , Nicolas Palix , linux-kernel@vger.kernel.org, cocci@systeme.lip6.fr Subject: Re: [PATCH] coccinelle: misc: remove "complex return code" warnings Message-ID: <20151001174718.GJ4284@localhost> References: <1443652647-23097-1-git-send-email-johan@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Oct 01, 2015 at 07:20:10AM +0200, Julia Lawall wrote: > On Wed, 30 Sep 2015, Johan Hovold wrote: > > > This effectively reverts 932058a5d5f9 ("coccinelle: misc: semantic patch > > to delete overly complex return code processing"). > > > > There can be both symmetry and readability reasons for not wanting to do > > the final function call as part of the return statement and to maintain > > a clear separation of success and error paths. > > > > Since this is in no way mandated by the coding standard, let's just > > remove this semantic patch to avoid having "clean up" patches being > > posted over and over in response to these Coccinelle warnings. > > What do you mean by "posted"? Are you referring to 0-day build testing > or individual usage of make coccicheck? Maybe it would make sense to > remove the semantic patch from 0-day build testing but leave it in the > kernel, perhaps removing the < 0 case because that one in practice doesn't > seem to turn up much that is useful? Individuals running coccicheck on in-kernel code and posting patches to "fix warnings", where the end result is not necessarily an improvement. But I don't think these warnings should be enabled for 0-day build testing either as it is should be up to the author to decide what style to prefer in each case. > Perhaps it could also be improved to detect a previous != 0 case and then > not return a warning. On some functions, this change can make some nice > simplifications. Yes, that would at least improve things. I don't think warnings should be generated at all for the following code: { int ret; ret = init_a(...); if (ret) return ret; ret = init_b(...); if (ret) return ret; return 0; } as it is (at least to me) preferred over: { int ret; ret = init_a(...); if (ret) return ret; return init_b(...); } for symmetry and readability reasons (e.g. I don't have to look at init_b to figure out what the functions returns). And with a long parameter list to init_b with line breaks, this would look even worse. But either way, it should be up to the author of the code to decide what style to use. Thanks, Johan