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

[Luben Tuikov]

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@xxxxxxxxxxx>
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@xxxxxxxxxxx>
    Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
    Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>

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