* checkpatch.pl: WARNING: else is not generally useful after a break or return @ 2020-04-17 19:20 Luben Tuikov 2020-04-17 19:56 ` Joe Perches 0 siblings, 1 reply; 3+ messages in thread From: Luben Tuikov @ 2020-04-17 19:20 UTC (permalink / raw) To: Joe Perches, Andy Whitcroft, LKML 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@perches.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@perches.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Linus Torvalds <torvalds@linux-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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: checkpatch.pl: WARNING: else is not generally useful after a break or return 2020-04-17 19:20 checkpatch.pl: WARNING: else is not generally useful after a break or return Luben Tuikov @ 2020-04-17 19:56 ` Joe Perches 2020-04-17 20:44 ` Luben Tuikov 0 siblings, 1 reply; 3+ messages in thread From: Joe Perches @ 2020-04-17 19:56 UTC (permalink / raw) To: Luben Tuikov, Andy Whitcroft, LKML 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@perches.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@perches.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Linus Torvalds <torvalds@linux-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. I disagree and do not believe this should be implemented in checkpatch as an accepted typical coding style. btw: Even in your example, amdgpu_device_gpu_recover has a return value, can fail, and likely should not just return 0. ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: checkpatch.pl: WARNING: else is not generally useful after a break or return 2020-04-17 19:56 ` Joe Perches @ 2020-04-17 20:44 ` Luben Tuikov 0 siblings, 0 replies; 3+ messages in thread From: Luben Tuikov @ 2020-04-17 20:44 UTC (permalink / raw) To: Joe Perches, Andy Whitcroft, LKML 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@perches.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@perches.com> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> >> Signed-off-by: Linus Torvalds <torvalds@linux-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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-04-17 20:44 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-04-17 19:20 checkpatch.pl: WARNING: else is not generally useful after a break or return Luben Tuikov 2020-04-17 19:56 ` Joe Perches 2020-04-17 20:44 ` Luben Tuikov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).