linux-kernel - Re: [PATCH] Documentation: checkpatch: Document some more message types

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: <CAKXUXMzEFQd1JJhx4ZbBQiuSB7Fk3bd7dwJYmPOvEtMqZopxpg@mail.gmail.com>
Date:   Wed, 29 Sep 2021 07:28:19 +0200
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Utkarsh Verma <utkarshverma294@...il.com>
Cc:     Dwaipayan Ray <dwaipayanray1@...il.com>,
        Joe Perches <joe@...ches.com>,
        Jonathan Corbet <corbet@....net>,
        "open list:DOCUMENTATION" <linux-doc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Documentation: checkpatch: Document some more message types

Overall conclusion: Patch needs more work. So a NACK from my side.

Jonathan, could you drop this patch from your queue again? Sorry for
this inconvenience.

Further comments inline.

On Sat, Sep 25, 2021 at 10:18 PM Utkarsh Verma
<utkarshverma294@...il.com> wrote:
>
> Added and documented 3 new message types:
> - UNNECESSARY_INT
> - UNSPECIFIED_INT
> - UNNECESSARY_ELSE
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@...il.com>
> ---
>  Documentation/dev-tools/checkpatch.rst | 47 ++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..2dc74682277f 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -929,6 +929,13 @@ Functions and Variables
>
>        return bar;
>
> +  **UNNECESSARY_INT**
> +    int used after short, long and long long is unnecessary. So remove it.
> +

This does not add significantly more explanation than what is already
there in the checkpatch warning without the --verbose option.

As we said multiple times before:
- A reference to documentation, mailing list thread, or (in this case)
even the section of the C standard helps. Then summarize that
discussion or the rationale you got from that documentation.
- Further, pointers to typical cases of false positives of this rule
also helps developers to judge if they should address the warning or
not.

> +  **UNSPECIFIED_INT**
> +    Kernel style prefers "unsigned int <foo>" over "unsigned <foo>" and
> +    "signed int <foo>" over "signed <foo>".
> +

Same comment as above.

>
>  Permissions
>  -----------
> @@ -1166,3 +1173,43 @@ Others
>
>    **TYPO_SPELLING**
>      Some words may have been misspelled.  Consider reviewing them.
> +
> +  **UNNECESSARY_ELSE**
> +    Using an else statement just after a return or a break statement is
> +    unnecassary. For example::

spelling mistake in unnecassary -> unnecessary.

> +
> +      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);
> +      }
> +
> +    So remove the else statement. But suppose if a if-else statement each
> +    with a single return statement, like::
> +
> +      if (foo)
> +              return bar;
> +      else
> +              return baz;
> +
> +    then by removing the else statement::
> +
> +      if (foo)
> +              return bar;
> +      return baz;
> +
> +    their is no significant increase in the readability and one can argue

s/their/there/

> +    that the first form is more readable because of indentation, so for
> +    such cases do not convert the existing code from first form to second
> +    form or vice-versa.

I am confused. So what is the recommendation the documentation is
providing here?

Lukas

> --
> 2.25.1
>