[Linux-kernel-mentees] [PATCH] checkpatch: add fix option and improve msg for BOOL_COMPARISON

[Lukas Bulwahn]

On Wed, Dec 9, 2020 at 4:59 PM Aditya <yashsri421 at gmail.com> wrote:
>
> On 9/12/20 5:17 pm, Lukas Bulwahn wrote:
> > On Wed, Dec 9, 2020 at 11:58 AM Aditya Srivastava <yashsri421 at gmail.com> wrote:
> >>
> >> Currently, checkpatch warns if the user compares expression with boolean.
> >>
> >> E.g., running checkpatch on commit 3b3a1a0b7824 ("staging: rtl8723bs:
> >> hal: Modify comparison to constant in rtl8723bs_xmit.c") reports:
> >>
> >> CHECK: Using comparison to true is error prone
> >> +       if (ret == true)
> >>
> >> Provide a fix by replacing the expression with preferred expression. i.e.,
> >>
> >> 1) If the check is for a 'true' or 'not false' expression, the operand
> >> will get replaced with empty string.
> >>
> >> 2) Else, the operand gets replaced with '!', and the arguments follow.
> >>
> >
> > Can you show the test cases you used to test this?
> >
> Yes, I ran it over certain commits.
>
> Eg 1) commit 5c789e131cbb ("netfilter: nf_conncount: Add list lock and
> gc worker, and RCU for init tree search"), where
>
> 1.i)
> +       if (list->dead == true) {
> gets changed to:
> +       if (list->dead) {
>
> 1.ii)
> +               if (rbconn->list.count == 0 && rbconn->list.dead == false) {
> gets changed to:
> +               if (rbconn->list.count == 0 && !rbconn->list.dead) {
>
> E.g. 2) commit 83ee6ec7740b75dc0db (Staging: rtl8723bs: os_dep: Fix
> if-else coding style issues) where
>
> 2.i)
> +       if (padapter->bDriverStopped == true) {
> gets changed to:
> +       if (padapter->bDriverStopped) {
>
> 2.ii)
> +       if (pwrpriv->bInSuspend == true) {
> gets changed to:
> +       if (pwrpriv->bInSuspend) {
>
> 2.iii)
> +       if (pwrpriv->bInSuspend == false) {
> gets changed to:
> +       if (!pwrpriv->bInSuspend) {
>
> > Can you show all findings of this rule on the current rc or linux-next?
> >
>
> I have generated report on last 10k commits of latest next branch,
> where the check was found to be reported 5 times.
>
5 times in 10k commits? I am really wondering how you select the fix
options you work on?

Do you consider this relevant when the number is so low?
Are there potentially other classes that are much more relevant instead?

Can you run it over all files in the repository instead of git commits?

Commits really do not help when we want to fix this in the current kernel.


Lukas

> Here are the generated msgs:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/bool_comparison/last_10k.txt
>
> I have also generated the new CHK messages for BOOL_COMPARISON (over
> v4.13..v5.8) at:
> https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/bool_comparison/commitsv4.13..v5.8.txt
>
> which might give us a better idea for what how the lines will be replaced.
>
> Thanks
> Aditya
>
> > Lukas
> >
> >> Suggested-by: Joe Perches <joe at perches.com>
> >> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> >> ---
> >> This fix is actually inspired from the comment mentioned in the script by Joe. I have added the Suggested-by line correspondingly.
> >>
> >>  scripts/checkpatch.pl | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> >> index 8bd8e4518f7f..abd5a3d2e913 100755
> >> --- a/scripts/checkpatch.pl
> >> +++ b/scripts/checkpatch.pl
> >> @@ -6973,8 +6973,11 @@ sub process {
> >>                                         $op = "";
> >>                                 }
> >>
> >> -                               CHK("BOOL_COMPARISON",
> >> -                                   "Using comparison to $otype is error prone\n" . $herecurr);
> >> +                               if (CHK("BOOL_COMPARISON",
> >> +                                       "Using comparison to '$otype' is error prone. Perhaps use '${lead}${op}${arg}${trail}'\n" . $herecurr) &&
> >> +                                   $fix) {
> >> +                                       $fixed[$fixlinenr] =~ s/\b(?:true|false|$Lval)\s*(?:==|\!=)\s*(?:true|false|$Lval)\b/$op$arg/;
> >> +                               }
> >>
> >>  ## maybe suggesting a correct construct would better
> >>  ##                                 "Using comparison to $otype is error prone.  Perhaps use '${lead}${op}${arg}${trail}'\n" . $herecurr);
> >> --
> >> 2.17.1
> >>
>