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

Aditya yashsri421 at gmail.com
Sat Feb 6 14:05:21 UTC 2021


On 13/12/20 1:33 pm, Lukas Bulwahn wrote:
> On Sat, Dec 12, 2020 at 10:13 AM Aditya <yashsri421 at gmail.com> wrote:
>>
>> On 9/12/20 10:39 pm, Lukas Bulwahn wrote:
>>> 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.
>>>
>>
>> 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?
>>
> 
> I suggest you submit a few patches correcting this style to a few
> driver maintainers to see if they are in favor of this checkpatch
> rule.
> 

Hi Lukas
I tried submitting the patches correcting this style, in various
subsystems as you suggested. Except staging/atomisp, every other
subsystem accepted the correction.

Do you think I should proceed with it?

Thanks
Aditya


More information about the Linux-kernel-mentees mailing list