linux-kernel - Re: [PATCH v2] docs: checkpatch: add UNNECESSARY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CABJPP5Bpc_0c=b8kymo0fgXZARNZJ8EM5F=Gpx1=y+vOfPWM8Q@mail.gmail.com>
Date:   Sun, 3 Oct 2021 10:08:17 +0530
From:   Dwaipayan Ray <dwaipayanray1@...il.com>
To:     Utkarsh Verma <utkarshverma294@...il.com>
Cc:     Lukas Bulwahn <lukas.bulwahn@...il.com>,
        Joe Perches <joe@...ches.com>,
        Jonathan Corbet <corbet@....net>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <utkarshverma294@...il.com> wrote:
>
> Added and documented UNNECESSARY_ELSE message type.
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@...il.com>
> ---
> Changes in v2:
>   - Included the continue statement.
>
>  Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..b7c41e876d1d 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -1166,3 +1166,80 @@ Others
>
>    **TYPO_SPELLING**
>      Some words may have been misspelled.  Consider reviewing them.
> +
> +  **UNNECESSARY_ELSE**
> +    Using an else statement just after a return/break/continue statement is
> +    unnecessary. For example::
> +
> +      for (i = 0; i < 100; i++) {
> +              int foo = bar();
> +              if (foo < 1)
> +                      break;
> +              else
> +                      usleep(1);
> +      }
> +
> +    is generally better written as::
> +
> +      for (i = 0; i < 100; i++) {
> +              int foo = bar();
> +              if (foo < 1)
> +                      break;
> +              usleep(1);
> +      }
> +
> +    It helps to reduce the indentation and removes the unnecessary else
> +    statement. But note, there can be some false positives because of the
> +    way it is implemented in the checkpatch script. The checkpatch script
> +    throws this warning message if it finds an else statement and the line
> +    above it is a break/continue/return statement indented at one tab more
> +    than the else statement. So there can be some false positives like::
> +
> +      int n = 15;
> +      if (n > 10)
> +              n--;
> +      else if (n == 10)
> +              return 0;
> +      else
> +              n++;
> +
> +    Now the checkpatch will give a warning for the use of else after return
> +    statement. If the else statement is removed then::
> +
> +      int n = 15;
> +      if (n > 10)
> +              n--;
> +      else if (n == 10)
> +              return 0;
> +      n++;
> +
> +    Now both the n-- and n++ statements will be executed which is different
> +    from the logic in the first case. As the if block doesn't have a return
> +    statement, so removing the else statement is wrong.
> +
> +    Always check the previous if/else if blocks, for break/continue/return
> +    statements, and do not blindly follow the checkpatch advice. One
> +    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
> +    even made it to the mainline, which was again reverted and fixed.
> +    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else

s/unnecesary/unnecessary
> +    after return statement.")
> +
> +    Also, do not change the code if there is only a single return statement
> +    inside if-else block, like::
> +
> +      if (a > b)
> +              return a;
> +      else
> +              return b;
> +
> +    now if the else statement is removed::
> +
> +      if (a > b)
> +              return a;
> +      return b;
> +
> +    there is no considerable increase in the readability and one can argue
> +    that the first form is more readable because of the indentation. So
> +    do not remove the else statement in case of a single return statement
> +    inside the if-else block.
> +    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
> --
> 2.25.1
>

I think this message is unnecessarily long for a warning that's understandable
at best without the verbose part. Try to shorten it up with only what's
required for a user to understand why the warning is there.

Dwaipayan.