linux-kernel - checkpatch.pl: WARNING: else is not generally useful after a break or return

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-next>] [day] [month] [year] [list]

Message-ID: <064ada88-9fa3-6f6d-967c-00d4e2d325ae@amd.com>
Date:   Fri, 17 Apr 2020 15:20:56 -0400
From:   Luben Tuikov <luben.tuikov@....com>
To:     Joe Perches <joe@...ches.com>, Andy Whitcroft <apw@...onical.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: checkpatch.pl: WARNING: else is not generally useful after a break or
 return

Hi,

I'm getting what seems to be a false positive in this case:

:32: WARNING: else is not generally useful after a break or return
#32: FILE: drivers/gpu/drm/amd/amdgpu/amdgpu_job.c:55:
+		return 0;
+	} else {

for the following code, at the bottom of a function:

	if (amdgpu_device_should_recover_gpu(ring->adev)) {
		amdgpu_device_gpu_recover(ring->adev, job);
		return 0;
	} else {
		drm_sched_suspend_timeout(&ring->sched);
		return 1;
	}
}

Which seems to be coming from commit:

commit 032a4c0f9a77ce565355c6e191553e853ba66f09
Author: Joe Perches <joe@...ches.com>
Date:   Wed Aug 6 16:10:29 2014 -0700

    checkpatch: warn on unnecessary else after return or break
    
    Using an else following a break or return can unnecessarily indent code
    blocks.
    
    ie:
            for (i = 0; i < 100; i++) {
                    int foo = bar();
                    if (foo < 1)
                            break;
                    else
                            usleep(1);
            }
    
    is generally better written as:
    
            for (i = 0; i < 100; i++) {
                    int foo = bar();
                    if (foo < 1)
                            break;
                    usleep(1);
            }
    
    Warn when a bare else statement is preceded by a break or return
    indented 1 tab more than the else.
    
    Signed-off-by: Joe Perches <joe@...ches.com>
    Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>

While I agree with what the commit is trying to do,
it doesn't seem to apply to the if-else statement which I quoted
above. That is, the "else" is not "bare"--to use the lingo of
the commit.

I suggest that no warning is issued when the "else" is a compound
statement, as shown in my example at the top of this email.

It is only natural to write:

	if (amdgpu_device_should_recover_gpu(ring->adev)) {
		amdgpu_device_gpu_recover(ring->adev, job);
		return 0;
	} else {
		drm_sched_suspend_timeout(&ring->sched);
		return 1;
	}
}

instead of,

	if (amdgpu_device_should_recover_gpu(ring->adev)) {
		amdgpu_device_gpu_recover(ring->adev, job);
		return 0;
	}
	drm_sched_suspend_timeout(&ring->sched);
	return 1;
}

Regards,
-- 
Luben