Re: checkpatch.pl warning for "return" with value

[Luben Tuikov]

On 2020-04-17 1:32 p.m., Joe Perches wrote:
> On Fri, 2020-04-17 at 13:20 -0400, Luben Tuikov wrote:
>> Hi guys,
>>
>> I get this warning:
>>
>> :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;
>> 	}
>> }
>>
>> It seems like a false positive--I mean, if the else branch was
>> taken, we'd return a different result.
> 
> There is an existing checkpatch exception for single line
> if/else returns
> like:
> 
> 	if (foo)
> 		return bar;
> 	else
> 		return baz;
> 
> because that's a pretty common code style.

Yes, that's true--and that's what I have above, except
the braces.

> 
> But I personally don't think that your example fits the
> same style.

Why? Linguistically, it doesn't matter if a compound
statement is used or a single one. (I mean, one could
use a comma "," too... :-) )

> 
> I think when unexpected condition should be separated from
> the expected condition which should typically be the last
> block of a function like:

I couldn't understand this sentence because of the "when"
and "which".

> 
> 
> 	if (<atypical_condition>) {
> 		...;
> 		return <atypical_result>;
> 	}
> 
> 	...;
> 	return <typical_result>;
> }
> 
> If you want to code it, and it works, go ahead, but I
> won't attempt it because I think it's not appropriate.

For error checking, when the 2nd ellipsis is
a long, long body of the function, so that the error
checking is done at the top, then long body,
then at the bottom we return some computed value.
But in the case I have above, it's a compact if-else
at the bottom of the function.

In the example I gave above, there is no "typical" or
"atypical" condition--it's just checking a condition
and deciding what to do, all at the bottom of
a function. (And that condition, samples
an external stimuli, which cannot be predicted.)

Also checking and returning from a function, doesn't
always have to be binary. It could be,

	if (A) {
		...;
		return X;
	} else if (B) {
		...;
		return Y;
	} else {
		...;
		return Z;
	}
}

And interestingly, checkpatch.pl doesn't complain for
the triplet above. But if I remove condition B, above,
it does complain.

Since we're returning a different result and since
it works fine with a triplet, could you fix the binary
case above to not complain?

You already seem to have an exception for it, as you stated
above.

Regards,
Luben