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@xxxxxxxxx> wrote:
>
> Added and documented 3 new message types:
> - UNNECESSARY_INT
> - UNSPECIFIED_INT
> - UNNECESSARY_ELSE
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@xxxxxxxxx>
> ---
> 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
>
