Re: [PATCH] Documentation: checkpatch: Document some more message types
From: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Date: 2021-09-29 05:28:25
Also in:
lkml
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 [off-list ref] wrote:
quoted hunk ↗ jump to hunk
Added and documented 3 new message types: - UNNECESSARY_INT - UNSPECIFIED_INT - UNNECESSARY_ELSE Signed-off-by: Utkarsh Verma <redacted> --- 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.
quoted hunk ↗ jump to hunk
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 argues/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