* [PATCH] Documentation: checkpatch: Document some more message types
@ 2021-09-25 16:38 Utkarsh Verma
0 siblings, 0 replies; 9+ messages in thread
From: Utkarsh Verma @ 2021-09-25 16:38 UTC (permalink / raw)
To: Dwaipayan Ray
Cc: Lukas Bulwahn, Joe Perches, Jonathan Corbet, linux-doc,
linux-kernel, Utkarsh Verma
Added and documented 3 new message types:
- MULTILINE_DEREFERENCE
- SINGLE_STATEMENT_DO_WHILE_MACRO
- MULTIPLE_ASSIGNMENTS
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
Documentation/dev-tools/checkpatch.rst | 43 ++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..dac5b89a3082 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -710,6 +710,33 @@ Indentation and Line Breaks
See: https://www.kernel.org/doc/html/latest/process/coding-style.html#breaking-long-lines-and-strings
+ **MULTILINE_DEREFERENCE**
+ A single dereferencing identifier spanned on multiple lines like::
+
+ struct_identifier->member[index].
+ member = <foo>;
+
+ is generally hard to follow. It can easily lead to typos and so makes
+ the code vulnerable to bugs.
+
+ If fixing the multiple line dereferencing leads to an 80 column
+ violation, then either rewrite the code in a more simple way or if the
+ starting part of the dereferencing identifier is the same and used at
+ multiple places then store it in a temporary variable, and use that
+ temporary variable only at all the places. For example, if there are
+ two dereferencing identifiers::
+
+ member1->member2->member3.foo1;
+ member1->member2->member3.foo2;
+
+ then store the member1->member2->member3 part in a temporary variable.
+ It not only helps to avoid the 80 column violation but also reduces
+ the program size by removing the unnecessary dereferences.
+
+ But if none of the above methods work then ignore the 80 column
+ violation because it is much easier to read a dereferencing identifier
+ on a single line.
+
**TRAILING_STATEMENTS**
Trailing statements (for example after any conditional) should be
on the next line.
@@ -845,6 +872,17 @@ Macros, Attributes and Symbols
Use the `fallthrough;` pseudo keyword instead of
`/* fallthrough */` like comments.
+ **SINGLE_STATEMENT_DO_WHILE_MACRO**
+ For the multi-statement macros, it is necessary to use the do-while
+ loop to avoid unpredictable code paths. The do-while loop helps to
+ group the multiple statements into a single one so that a
+ function-like macro can be used as a function only.
+
+ But for the single statement macros, it is unnecessary to use the
+ do-while loop. Although the code is syntactically correct but using
+ the do-while loop is redundant. So remove the do-while loop for single
+ statement macros.
+
**WEAK_DECLARATION**
Using weak declarations like __attribute__((weak)) or __weak
can have unintended link defects. Avoid using them.
@@ -920,6 +958,11 @@ Functions and Variables
Your compiler (or rather your loader) automatically does
it for you.
+ **MULTIPLE_ASSIGNMENTS**
+ Multiple assignments on a single line makes the code unnecessarily
+ complicated. So on a single line assign value to a single variable
+ only, this makes the code more readable and helps avoid typos.
+
**RETURN_PARENTHESES**
return is not a function and as such doesn't need parentheses::
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] Documentation: checkpatch: Document some more message types
@ 2021-09-25 20:17 Utkarsh Verma
2021-09-27 17:43 ` Jonathan Corbet
2021-09-29 5:28 ` Lukas Bulwahn
0 siblings, 2 replies; 9+ messages in thread
From: Utkarsh Verma @ 2021-09-25 20:17 UTC (permalink / raw)
To: Dwaipayan Ray
Cc: Lukas Bulwahn, Joe Perches, Jonathan Corbet, linux-doc,
linux-kernel, Utkarsh Verma
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.
+
+ **UNSPECIFIED_INT**
+ Kernel style prefers "unsigned int <foo>" over "unsigned <foo>" and
+ "signed int <foo>" over "signed <foo>".
+
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::
+
+ 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
+ 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.
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Document some more message types
2021-09-25 20:17 Utkarsh Verma
@ 2021-09-27 17:43 ` Jonathan Corbet
2021-09-27 17:47 ` Utkarsh Verma
2021-09-27 17:53 ` Joe Perches
2021-09-29 5:28 ` Lukas Bulwahn
1 sibling, 2 replies; 9+ messages in thread
From: Jonathan Corbet @ 2021-09-27 17:43 UTC (permalink / raw)
To: Utkarsh Verma, Dwaipayan Ray
Cc: Lukas Bulwahn, Joe Perches, linux-doc, linux-kernel, Utkarsh Verma
Utkarsh Verma <utkarshverma294@gmail.com> writes:
> 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(+)
So...when you send multiple patches with the same subject line that's
always a bad sign. We really want a "git --oneline" listing to give a
good idea of what the patch does, and that depends on more descriptive
subject lines.
In this case, something like:
docs: checkpatch: add UNNECESSARY/UNSPECIFIED_INT and UNNECESSARY_ELSE
I can fix up these two patches, but please try to keep this in mind for
future work.
(applying the patches now).
Thanks,
jon
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Document some more message types
2021-09-27 17:43 ` Jonathan Corbet
@ 2021-09-27 17:47 ` Utkarsh Verma
2021-09-27 17:53 ` Joe Perches
1 sibling, 0 replies; 9+ messages in thread
From: Utkarsh Verma @ 2021-09-27 17:47 UTC (permalink / raw)
To: Jonathan Corbet, Dwaipayan Ray
Cc: Lukas Bulwahn, Joe Perches, linux-doc, linux-kernel
On Mon, Sep 27, 2021 at 11:43:59AM -0600, Jonathan Corbet wrote:
> Utkarsh Verma <utkarshverma294@gmail.com> writes:
>
> > 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(+)
>
> So...when you send multiple patches with the same subject line that's
> always a bad sign. We really want a "git --oneline" listing to give a
> good idea of what the patch does, and that depends on more descriptive
> subject lines.
>
> In this case, something like:
>
> docs: checkpatch: add UNNECESSARY/UNSPECIFIED_INT and UNNECESSARY_ELSE
>
> I can fix up these two patches, but please try to keep this in mind for
> future work.
>
Alright, I will keep this in mind.
> (applying the patches now).
>
> Thanks,
>
> jon
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Document some more message types
2021-09-27 17:43 ` Jonathan Corbet
2021-09-27 17:47 ` Utkarsh Verma
@ 2021-09-27 17:53 ` Joe Perches
2021-10-01 19:09 ` Utkarsh Verma
1 sibling, 1 reply; 9+ messages in thread
From: Joe Perches @ 2021-09-27 17:53 UTC (permalink / raw)
To: Jonathan Corbet, Utkarsh Verma, Dwaipayan Ray
Cc: Lukas Bulwahn, linux-doc, linux-kernel
On Mon, 2021-09-27 at 11:43 -0600, Jonathan Corbet wrote:
> Utkarsh Verma <utkarshverma294@gmail.com> writes:
>
> > 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(+)
>
> So...when you send multiple patches with the same subject line that's
> always a bad sign. We really want a "git --oneline" listing to give a
> good idea of what the patch does, and that depends on more descriptive
> subject lines.
>
> In this case, something like:
>
> docs: checkpatch: add UNNECESSARY/UNSPECIFIED_INT and UNNECESSARY_ELSE
>
> I can fix up these two patches, but please try to keep this in mind for
> future work.
>
> (applying the patches now).
The unnecessary_else description isn't particularly good as the
checkpatch output doesn't describe multiple if/else if/else if type
returns where the message should not apply.
For this type of use, the checkpatch message is not necessarily correct
and because it could be a patch context, there's no way for checkpatch
to know if it's correct or not.
if (foo) {
...
} else if (bar) {
...
return [val];
} else {
...
}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Document some more message types
2021-09-27 17:53 ` Joe Perches
@ 2021-10-01 19:09 ` Utkarsh Verma
0 siblings, 0 replies; 9+ messages in thread
From: Utkarsh Verma @ 2021-10-01 19:09 UTC (permalink / raw)
To: Joe Perches, Jonathan Corbet, Dwaipayan Ray
Cc: Lukas Bulwahn, linux-doc, linux-kernel
On Mon, Sep 27, 2021 at 10:53:05AM -0700, Joe Perches wrote:
> On Mon, 2021-09-27 at 11:43 -0600, Jonathan Corbet wrote:
> > Utkarsh Verma <utkarshverma294@gmail.com> writes:
> >
> > > 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(+)
> >
> > So...when you send multiple patches with the same subject line that's
> > always a bad sign. We really want a "git --oneline" listing to give a
> > good idea of what the patch does, and that depends on more descriptive
> > subject lines.
> >
> > In this case, something like:
> >
> > docs: checkpatch: add UNNECESSARY/UNSPECIFIED_INT and UNNECESSARY_ELSE
> >
> > I can fix up these two patches, but please try to keep this in mind for
> > future work.
> >
> > (applying the patches now).
>
> The unnecessary_else description isn't particularly good as the
> checkpatch output doesn't describe multiple if/else if/else if type
> returns where the message should not apply.
>
> For this type of use, the checkpatch message is not necessarily correct
> and because it could be a patch context, there's no way for checkpatch
> to know if it's correct or not.
>
> if (foo) {
> ...
> } else if (bar) {
> ...
> return [val];
> } else {
> ...
> }
>
Sorry, my bad. I have sent a new patch for the UNNECESSARY_ELSE test.
So please do review it.
Maybe we should add a check for the continue statement also, because it is
similar to the break and return statements, and using else after continue
statement is unnecessary.
Regards,
Utkarsh Verma
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Document some more message types
2021-09-25 20:17 Utkarsh Verma
2021-09-27 17:43 ` Jonathan Corbet
@ 2021-09-29 5:28 ` Lukas Bulwahn
1 sibling, 0 replies; 9+ messages in thread
From: Lukas Bulwahn @ 2021-09-29 5:28 UTC (permalink / raw)
To: Utkarsh Verma
Cc: Dwaipayan Ray, Joe Perches, Jonathan Corbet,
open list:DOCUMENTATION, Linux Kernel Mailing List
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] Documentation: checkpatch: Document some more message types
@ 2021-09-17 10:15 Utkarsh Verma
2021-09-17 12:39 ` Dwaipayan Ray
0 siblings, 1 reply; 9+ messages in thread
From: Utkarsh Verma @ 2021-09-17 10:15 UTC (permalink / raw)
To: Dwaipayan Ray
Cc: Lukas Bulwahn, Joe Perches, Jonathan Corbet, linux-doc,
linux-kernel, Utkarsh Verma
Added and documented 4 new message types:
- INCLUDE_LINUX
- INDENTED_LABEL
- IF_0
- IF_1
Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
Documentation/dev-tools/checkpatch.rst | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..ea343a7a5b46 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -435,6 +435,11 @@ API usage
**EXPORT_SYMBOL**
EXPORT_SYMBOL should immediately follow the symbol to be exported.
+ **INCLUDE_LINUX**
+ Whenever asm/file.h is included and linux/file.h exists, a
+ conversion can be made when linux/file.h includes asm/file.h.
+ However this is not always the case (See signal.h).
+
**IN_ATOMIC**
in_atomic() is not for driver use so any such use is reported as an ERROR.
Also in_atomic() is often used to determine if sleeping is permitted,
@@ -661,6 +666,10 @@ Indentation and Line Breaks
See: https://lore.kernel.org/lkml/1328311239.21255.24.camel@joe2Laptop/
+ **INDENTED_LABEL**
+ goto labels either should not have any indentation or only a single
+ space indentation.
+
**SWITCH_CASE_INDENT_LEVEL**
switch should be at the same indent as case.
Example::
@@ -790,6 +799,19 @@ Macros, Attributes and Symbols
**DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON**
do {} while(0) macros should not have a trailing semicolon.
+ **IF_0**
+ The code enclosed within #if 0 and #endif is not executed and is used
+ for temporarily removing the segments of code with the intention of
+ using it in the future, much like comments. But comments cannot be
+ nested, so #if 0 is preferred. But if the code inside #if 0 and #endif
+ doesn't seem to be anymore required then remove it.
+
+ **IF_1**
+ The code enclosed within #if 1 and #endif is always executed, so the
+ #if 1 and #endif statements are redundant, thus remove it.
+ It is only useful for debugging purposes, it can quickly disable the
+ code enclosed within itself by changing #if 1 to #if 0
+
**INIT_ATTRIBUTE**
Const init definitions should use __initconst instead of
__initdata.
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] Documentation: checkpatch: Document some more message types
2021-09-17 10:15 Utkarsh Verma
@ 2021-09-17 12:39 ` Dwaipayan Ray
0 siblings, 0 replies; 9+ messages in thread
From: Dwaipayan Ray @ 2021-09-17 12:39 UTC (permalink / raw)
To: Utkarsh Verma
Cc: Lukas Bulwahn, Joe Perches, Jonathan Corbet,
open list:DOCUMENTATION, linux-kernel
On Fri, Sep 17, 2021 at 3:46 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Added and documented 4 new message types:
> - INCLUDE_LINUX
> - INDENTED_LABEL
> - IF_0
> - IF_1
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
> Documentation/dev-tools/checkpatch.rst | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..ea343a7a5b46 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -435,6 +435,11 @@ API usage
> **EXPORT_SYMBOL**
> EXPORT_SYMBOL should immediately follow the symbol to be exported.
>
> + **INCLUDE_LINUX**
> + Whenever asm/file.h is included and linux/file.h exists, a
> + conversion can be made when linux/file.h includes asm/file.h.
> + However this is not always the case (See signal.h).
> +
Can you suggest why? And is this true for every use?
> **IN_ATOMIC**
> in_atomic() is not for driver use so any such use is reported as an ERROR.
> Also in_atomic() is often used to determine if sleeping is permitted,
> @@ -661,6 +666,10 @@ Indentation and Line Breaks
>
> See: https://lore.kernel.org/lkml/1328311239.21255.24.camel@joe2Laptop/
>
> + **INDENTED_LABEL**
> + goto labels either should not have any indentation or only a single
> + space indentation.
> +
Some reference here maybe?
> **SWITCH_CASE_INDENT_LEVEL**
> switch should be at the same indent as case.
> Example::
> @@ -790,6 +799,19 @@ Macros, Attributes and Symbols
> **DO_WHILE_MACRO_WITH_TRAILING_SEMICOLON**
> do {} while(0) macros should not have a trailing semicolon.
>
> + **IF_0**
> + The code enclosed within #if 0 and #endif is not executed and is used
> + for temporarily removing the segments of code with the intention of
> + using it in the future, much like comments. But comments cannot be
> + nested, so #if 0 is preferred. But if the code inside #if 0 and #endif
> + doesn't seem to be anymore required then remove it.
> +
> + **IF_1**
> + The code enclosed within #if 1 and #endif is always executed, so the
> + #if 1 and #endif statements are redundant, thus remove it.
> + It is only useful for debugging purposes, it can quickly disable the
> + code enclosed within itself by changing #if 1 to #if 0
> +
These two are good.
> **INIT_ATTRIBUTE**
> Const init definitions should use __initconst instead of
> __initdata.
> --
> 2.25.1
>
Thanks,
Dwaipayan.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-01 19:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-25 16:38 [PATCH] Documentation: checkpatch: Document some more message types Utkarsh Verma
-- strict thread matches above, loose matches on Subject: below --
2021-09-25 20:17 Utkarsh Verma
2021-09-27 17:43 ` Jonathan Corbet
2021-09-27 17:47 ` Utkarsh Verma
2021-09-27 17:53 ` Joe Perches
2021-10-01 19:09 ` Utkarsh Verma
2021-09-29 5:28 ` Lukas Bulwahn
2021-09-17 10:15 Utkarsh Verma
2021-09-17 12:39 ` Dwaipayan Ray
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.