linux-kernel - Re: [PATCH 29/29] tools: Avoid comma separated statements

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6011591.XMClsHuqKX@c100>
Date:   Wed, 26 Aug 2020 11:30:14 +0200
From:   Thomas Renninger <trenn@...e.com>
To:     Jiri Kosina <trivial@...nel.org>, linux-kernel@...r.kernel.org,
        Joe Perches <joe@...ches.com>
Cc:     Shuah Khan <shuah@...nel.org>, linux-pm@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH 29/29] tools: Avoid comma separated statements

Hi,

getting rid of lines with multiple instructions, separated by comma is
certainly a good idea.
One nit pick, though:

Am Dienstag, 25. August 2020, 06:56:26 CEST schrieb Joe Perches:
> Use semicolons and braces.
> 
> Signed-off-by: Joe Perches <joe@...ches.com>
> ---
>  tools/lib/subcmd/help.c                    |  10 +-
>  tools/power/cpupower/utils/cpufreq-set.c   |  14 +-
>  tools/testing/selftests/vm/gup_benchmark.c |  18 +-
>  tools/testing/selftests/vm/userfaultfd.c   | 296 +++++++++++++--------
>  4 files changed, 210 insertions(+), 128 deletions(-)
> 
> diff --git a/tools/lib/subcmd/help.c b/tools/lib/subcmd/help.c
> index 2859f107abc8..bf02d62a3b2b 100644
> --- a/tools/lib/subcmd/help.c
> +++ b/tools/lib/subcmd/help.c
> @@ -65,12 +65,14 @@ void exclude_cmds(struct cmdnames *cmds, struct cmdnames
> *excludes) ci = cj = ei = 0;
>  	while (ci < cmds->cnt && ei < excludes->cnt) {
>  		cmp = strcmp(cmds->names[ci]->name, excludes->names[ei]->name);
> -		if (cmp < 0)
> +		if (cmp < 0) {
>  			cmds->names[cj++] = cmds->names[ci++];
> -		else if (cmp == 0)
> -			ci++, ei++;
> -		else if (cmp > 0)
> +		} else if (cmp == 0) {
> +			ci++;
>  			ei++;
> +		} else if (cmp > 0) {
> +			ei++;
> +		}
>  	}

I can remember patches being rejected with one line statements in a condition,
surounded by braces.
I just read up Documentation/process/coding-style.rst, to be sure this still is up-to-date.
It's not a must, but line 180 says:
"Do not unnecessarily use braces where a single statement will do."

So if this is about coding style cleanup, IMO you should remove braces from single line
statements.

I haven't reviewed every line, but I expect you only split up comma separated instructions
into separate lines and added braces?

Afaik there isn't a specific tag, but having:
cleanup only: No functional change

in the changelog would be nice for people looking for fixes to backport.

Otherwise, I think this is a worthful cleanup.

     Thomas