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

Lukas Bulwahn lukas.bulwahn at gmail.com
Sun Dec 13 08:03:46 UTC 2020


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.

Lukas

> Thanks
> Aditya


More information about the Linux-kernel-mentees mailing list