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

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

[Joe Perches]

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.

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

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


	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.

cheers, Joe

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

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

[Joe Perches]

On Fri, 2020-04-17 at 14:52 -0400, Luben Tuikov wrote:
> On 2020-04-17 1:32 p.m., Joe Perches wrote:
[]
> > 	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.

which could not be differentiated from a function like

{
	...;
	err = func(...);
	if (err) {
		report("err: %d\n", err);
		return err;
	}

	...;
	return 0;

> 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.)
[]
> 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?

No.

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

[Joe Perches]

On Fri, 2020-04-17 at 14:52 -0400, Luben Tuikov wrote:
> 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.

My recollection is false positives existed so the
else if case was not wanted.