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

Aditya yashsri421 at gmail.com
Wed Dec 9 15:59:48 UTC 2020


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.

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



More information about the Linux-kernel-mentees mailing list