Messages in this thread | | | From | Lukas Bulwahn <> | Date | Wed, 29 Sep 2021 07:28:19 +0200 | 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@gmail.com> wrote: > > Added and documented 3 new message types: > - UNNECESSARY_INT > - UNSPECIFIED_INT > - UNNECESSARY_ELSE > > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.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 >
| |