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

[Aditya <yashsri421@gmail.com>]

On 9/12/20 10:39 pm, Lukas Bulwahn wrote:
> On Wed, Dec 9, 2020 at 4:59 PM Aditya <yashsri421@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@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.
> 

Hi
So, I ran  grep -e
"==\s*true\|==\s*false\|!=\s*true\|!=\s*false\|true\s*==\|false\s*==\|true\s*!=\|false\s*!="
. -R over the repo.
We currently have ~1630 such occurrences in different files.

Detail findings can be found here:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/bool_comparison/file_results.txt

A large amount of these occurrences are found in drivers/, with
"drivers/gpu/drm/amd/display/dc/dml/" having most occurrences.

What do you think?

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees