Message-ID: <54cef727-b637-a738-3c5a-a25d36b9fae5@amd.com>
Date:   Fri, 17 Apr 2020 16:44:02 -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: Re: checkpatch.pl: WARNING: else is not generally useful after a
 break or return

On 2020-04-17 3:56 p.m., Joe Perches wrote:
> On Fri, 2020-04-17 at 15:20 -0400, Luben Tuikov wrote:
>> 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;
>> }
> 
> This is continuing an email thread sent privately to Andy and me.

To which you replied to the list and snipped some portions.
In this thread, I include your commit which shows the intention
of the check, and add more clarification as to what the problem is,
with more examples, including your example from your commit message.

> I disagree and do not believe this should be implemented in
> checkpatch as an accepted typical coding style.

So you'd force everyone to write:

	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;
}

Instead of the more natural,

	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;
	}
}

I believe that checkpatch.pl shouldn't force to break
up a natural if-else as shown immediately above, but
allow a user to use that type of expression.

The intention of the original commit is fine, and it gave
an example of what it is trying to fix, with an example
in the commit message, but it gives a false-positive
for the code snippet above. All I'm asking is for this
particular scenario to be fixed in checkpatch.pl,
so that people could write, at the bottom of a function:

	if (cond_A) {
		do_A();
		return 0;
	} else {
		do_B();
		return 1;
	}
}

instead of the more obscure (forced by current checkpatch.pl),

	if (cond_A) {
		do_A();
		return 0;
	}
	do_B();
	return 1;
}

In your commit message example, you have a jump statement,
"break", in one path, and a non-jump statement in the other:

    if (X)                  if (X)
        break;                  break;
    else           ===>     usleep();
        usleep();

However, in the false-positive example, both paths
have a return with a value:

	if (X) {
		do_X();
		return 0;
	} else {
		do_Y();
		return 1;
	}

Which is slightly more complex than the "break; else" example
given in the commit message of the original commit, and shouldn't
generate a "WARNING".

You cannot blindly apply
	If "return" followed by "else", then WARNING.
rule. It is slightly more complicated than that.

> btw:
> 
> Even in your example, amdgpu_device_gpu_recover has a return
> value, can fail, and likely should not just return 0.

Joe, that's work in progress. How do you know if amdgpu_device_gpu_recover()
wasn't modified to void?

Regards,
Luben

Powered by blists - more mailing lists