[PATCH] Documentation: checkpatch: Document some more message types

[PATCH] Documentation: checkpatch: Document some more message types

[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

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

[Jonathan Corbet]

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

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

[Utkarsh Verma]

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

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

[Joe Perches]

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 {
		...
	}

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

[Lukas Bulwahn]

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
>

[PATCH] docs: checkpatch: add UNNECESSARY_ELSE message

[Utkarsh Verma]

Added and documented UNNECESSARY_ELSE message type.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..e0f1ea1a0911 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -1166,3 +1166,80 @@ 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
+    unnecessary. 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);
+      }
+
+    It helps to reduce the indentation and removes the unnecessary else
+    statement. But note there can be some false positives because of the
+    way it is implemented in the checkpatch script. The checkpatch script
+    throws this warning message if it finds an else statement and the line
+    above it is a break or return statement indented at one tab more than
+    the else statement. So there can be some false positives like::
+
+      int n = 15;
+      if (n > 10)
+              n--;
+      else if (n == 10)
+              return 0;
+      else
+              n++;
+
+    Now the checkpatch will give a warning for the use of else after return
+    statement. If the else statement is removed then::
+
+      int n = 15;
+      if (n > 10)
+              n--;
+      else if (n == 10)
+              return 0;
+      n++;
+
+    Now both the n-- and n++ statements will be executed which is different
+    from the logic in the first case. Because the if block doesn't have
+    a return statement, so removing the else statement is wrong.
+
+    Always check the previous if/else if blocks, for break/return
+    statements, and do not blindly follow the checkpatch advice. One
+    patch https://lore.kernel.org/all/20200615155131.GA4563@sevic69/
+    even made it to the mainline, which was again reverted and fixed.
+    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else
+    after return statement.")
+
+    Also, do not change the code if there is only a single return/break
+    statement inside if-else block, like::
+
+      if (a > b)
+              return a;
+      else
+              return b;
+
+    now if the else statement is removed::
+
+      if (a > b)
+              return a;
+      return b;
+
+    there is no considerable increase in the readability and one can argue
+    that the first form is more readable because of the indentation. So
+    do not remove the else statement in case of a single return/break
+    statements inside the if-else block.
+    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
-- 
2.25.1

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

[Utkarsh Verma]

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

[PATCH] checkpatch: add check for continue statement in UNNECESSARY_ELSE

[Utkarsh Verma]

UNNECESSARY_ELSE only checks for the usage of else after a return or
break. But the same logic is also true for continue statement.

else used after a continue statement is unnecessary. So add a test
for continue statement also.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 scripts/checkpatch.pl | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c27d2312cfc3..f5a911aa6b64 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4011,15 +4011,16 @@ sub process {
 
 # check indentation of any line with a bare else
 # (but not if it is a multiple line "if (foo) return bar; else return baz;")
-# if the previous line is a break or return and is indented 1 tab more...
+# if the previous line is a break or continue or return and is indented 1 tab more...
 		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
 			my $tabs = length($1) + 1;
 			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
+			    $prevline =~ /^\+\t{$tabs,$tabs}continue\b/ ||
 			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
 			     defined $lines[$linenr] &&
 			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
 				WARN("UNNECESSARY_ELSE",
-				     "else is not generally useful after a break or return\n" . $hereprev);
+				     "else is not generally useful after a break or continue or return\n" . $hereprev);
 			}
 		}
 
-- 
2.25.1

Re: [PATCH] checkpatch: add check for continue statement in UNNECESSARY_ELSE

[Joe Perches]

On Sat, 2021-10-02 at 00:56 +0530, Utkarsh Verma wrote:
> UNNECESSARY_ELSE only checks for the usage of else after a return or
> break. But the same logic is also true for continue statement.
> 
> else used after a continue statement is unnecessary. So add a test
> for continue statement also.
> 
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  scripts/checkpatch.pl | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c27d2312cfc3..f5a911aa6b64 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4011,15 +4011,16 @@ sub process {
>  
> 
>  # check indentation of any line with a bare else
>  # (but not if it is a multiple line "if (foo) return bar; else return baz;")
> -# if the previous line is a break or return and is indented 1 tab more...
> +# if the previous line is a break or continue or return and is indented 1 tab more...
>  		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
>  			my $tabs = length($1) + 1;
>  			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
> +			    $prevline =~ /^\+\t{$tabs,$tabs}continue\b/ ||

I suppose this is ok.  I'd generally write this on one line.

			if ($prevline =~ /^\+\t{$tabs,$tabs}(?:break|continue)\b/ ||

[PATCH v2] checkpatch: add check for continue statement in UNNECESSARY_ELSE

[Utkarsh Verma]

UNNECESSARY_ELSE only checks for the usage of else after a return or
break. But the same logic is also true for continue statement.

else used after a continue statement is unnecessary. So add a test
for continue statement also.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c27d2312cfc3..edbb74dda5cb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4011,15 +4011,15 @@ sub process {
 
 # check indentation of any line with a bare else
 # (but not if it is a multiple line "if (foo) return bar; else return baz;")
-# if the previous line is a break or return and is indented 1 tab more...
+# if the previous line is a break or continue or return and is indented 1 tab more...
 		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
 			my $tabs = length($1) + 1;
-			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
+			if ($prevline =~ /^\+\t{$tabs,$tabs}(?:break|continue)\b/ ||
 			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
 			     defined $lines[$linenr] &&
 			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
 				WARN("UNNECESSARY_ELSE",
-				     "else is not generally useful after a break or return\n" . $hereprev);
+				     "else is not generally useful after a break or continue or return\n" . $hereprev);
 			}
 		}
 
-- 
2.25.1

Re: [PATCH v2] checkpatch: add check for continue statement in UNNECESSARY_ELSE

[Joe Perches]

On Sat, 2021-10-02 at 02:17 +0530, Utkarsh Verma wrote:
> UNNECESSARY_ELSE only checks for the usage of else after a return or
> break. But the same logic is also true for continue statement.
> 
> else used after a continue statement is unnecessary. So add a test
> for continue statement also.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -4011,15 +4011,15 @@ sub process {
>  
> 
>  # check indentation of any line with a bare else
>  # (but not if it is a multiple line "if (foo) return bar; else return baz;")
> -# if the previous line is a break or return and is indented 1 tab more...
> +# if the previous line is a break or continue or return and is indented 1 tab more...
>  		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
>  			my $tabs = length($1) + 1;
> -			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
> +			if ($prevline =~ /^\+\t{$tabs,$tabs}(?:break|continue)\b/ ||
>  			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
>  			     defined $lines[$linenr] &&
>  			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
>  				WARN("UNNECESSARY_ELSE",
> -				     "else is not generally useful after a break or return\n" . $hereprev);
> +				     "else is not generally useful after a break or continue or return\n" . $hereprev);
>  			}
>  		}
>  
> 

Maybe make the output specific to the break/continue/return
(untested)

			if ($prevline =~ /^\+\t{$tabs,$tabs}(break|continue|return)\b/ &&
			    !($1 == "return" &&
			      defined $lines[$linenr] &&
			      $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
				WARN("UNNECESSARY_ELSE",
				     "else is not generally useful after a $1\n" . $hereprev);

[PATCH v3] checkpatch: add check for continue statement in UNNECESSARY_ELSE

[Utkarsh Verma]

UNNECESSARY_ELSE only checks for the usage of else after a return or
break. But the same logic is also true for continue statement.

else used after a continue statement is unnecessary. So add a test
for continue statement also.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
 scripts/checkpatch.pl | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c27d2312cfc3..0eee086d87fe 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4011,15 +4011,15 @@ sub process {
 
 # check indentation of any line with a bare else
 # (but not if it is a multiple line "if (foo) return bar; else return baz;")
-# if the previous line is a break or return and is indented 1 tab more...
+# if the previous line is a break or continue or return and is indented 1 tab more...
 		if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
 			my $tabs = length($1) + 1;
-			if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
-			    ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
-			     defined $lines[$linenr] &&
-			     $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
+			if ($prevline =~ /^\+\t{$tabs,$tabs}(break|continue|return)\b/ &&
+			    !($1 eq "return" &&
+			      defined $lines[$linenr] &&
+			      $lines[$linenr] =~ /^[ \+]\t{$tabs,$tabs}return/)) {
 				WARN("UNNECESSARY_ELSE",
-				     "else is not generally useful after a break or return\n" . $hereprev);
+				     "else is not generally useful after a $1\n" . $hereprev);
 			}
 		}
 
-- 
2.25.1

[PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

[Utkarsh Verma]

Added and documented UNNECESSARY_ELSE message type.

Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
---
Changes in v2:
  - Included the continue statement.

 Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index f0956e9ea2d8..b7c41e876d1d 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -1166,3 +1166,80 @@ Others
 
   **TYPO_SPELLING**
     Some words may have been misspelled.  Consider reviewing them.
+
+  **UNNECESSARY_ELSE**
+    Using an else statement just after a return/break/continue statement is
+    unnecessary. 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);
+      }
+
+    It helps to reduce the indentation and removes the unnecessary else
+    statement. But note, there can be some false positives because of the
+    way it is implemented in the checkpatch script. The checkpatch script
+    throws this warning message if it finds an else statement and the line
+    above it is a break/continue/return statement indented at one tab more
+    than the else statement. So there can be some false positives like::
+
+      int n = 15;
+      if (n > 10)
+              n--;
+      else if (n == 10)
+              return 0;
+      else
+              n++;
+
+    Now the checkpatch will give a warning for the use of else after return
+    statement. If the else statement is removed then::
+
+      int n = 15;
+      if (n > 10)
+              n--;
+      else if (n == 10)
+              return 0;
+      n++;
+
+    Now both the n-- and n++ statements will be executed which is different
+    from the logic in the first case. As the if block doesn't have a return
+    statement, so removing the else statement is wrong.
+
+    Always check the previous if/else if blocks, for break/continue/return
+    statements, and do not blindly follow the checkpatch advice. One
+    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
+    even made it to the mainline, which was again reverted and fixed.
+    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else
+    after return statement.")
+
+    Also, do not change the code if there is only a single return statement
+    inside if-else block, like::
+
+      if (a > b)
+              return a;
+      else
+              return b;
+
+    now if the else statement is removed::
+
+      if (a > b)
+              return a;
+      return b;
+
+    there is no considerable increase in the readability and one can argue
+    that the first form is more readable because of the indentation. So
+    do not remove the else statement in case of a single return statement
+    inside the if-else block.
+    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
-- 
2.25.1

Re: [PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

[Dwaipayan Ray]

On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Added and documented UNNECESSARY_ELSE message type.
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
> Changes in v2:
>   - Included the continue statement.
>
>  Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..b7c41e876d1d 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -1166,3 +1166,80 @@ Others
>
>    **TYPO_SPELLING**
>      Some words may have been misspelled.  Consider reviewing them.
> +
> +  **UNNECESSARY_ELSE**
> +    Using an else statement just after a return/break/continue statement is
> +    unnecessary. 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);
> +      }
> +
> +    It helps to reduce the indentation and removes the unnecessary else
> +    statement. But note, there can be some false positives because of the
> +    way it is implemented in the checkpatch script. The checkpatch script
> +    throws this warning message if it finds an else statement and the line
> +    above it is a break/continue/return statement indented at one tab more
> +    than the else statement. So there can be some false positives like::
> +
> +      int n = 15;
> +      if (n > 10)
> +              n--;
> +      else if (n == 10)
> +              return 0;
> +      else
> +              n++;
> +
> +    Now the checkpatch will give a warning for the use of else after return
> +    statement. If the else statement is removed then::
> +
> +      int n = 15;
> +      if (n > 10)
> +              n--;
> +      else if (n == 10)
> +              return 0;
> +      n++;
> +
> +    Now both the n-- and n++ statements will be executed which is different
> +    from the logic in the first case. As the if block doesn't have a return
> +    statement, so removing the else statement is wrong.
> +
> +    Always check the previous if/else if blocks, for break/continue/return
> +    statements, and do not blindly follow the checkpatch advice. One
> +    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
> +    even made it to the mainline, which was again reverted and fixed.
> +    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else

s/unnecesary/unnecessary
> +    after return statement.")
> +
> +    Also, do not change the code if there is only a single return statement
> +    inside if-else block, like::
> +
> +      if (a > b)
> +              return a;
> +      else
> +              return b;
> +
> +    now if the else statement is removed::
> +
> +      if (a > b)
> +              return a;
> +      return b;
> +
> +    there is no considerable increase in the readability and one can argue
> +    that the first form is more readable because of the indentation. So
> +    do not remove the else statement in case of a single return statement
> +    inside the if-else block.
> +    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
> --
> 2.25.1
>

I think this message is unnecessarily long for a warning that's understandable
at best without the verbose part. Try to shorten it up with only what's
required for a user to understand why the warning is there.

Dwaipayan.

Re: [PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

[Lukas Bulwahn]

On Sun, Oct 3, 2021 at 6:38 AM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > Added and documented UNNECESSARY_ELSE message type.
> >
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > ---
> > Changes in v2:
> >   - Included the continue statement.
> >
> >  Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index f0956e9ea2d8..b7c41e876d1d 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -1166,3 +1166,80 @@ Others
> >
> >    **TYPO_SPELLING**
> >      Some words may have been misspelled.  Consider reviewing them.
> > +
> > +  **UNNECESSARY_ELSE**
> > +    Using an else statement just after a return/break/continue statement is
> > +    unnecessary. 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);
> > +      }
> > +
> > +    It helps to reduce the indentation and removes the unnecessary else
> > +    statement. But note, there can be some false positives because of the
> > +    way it is implemented in the checkpatch script. The checkpatch script
> > +    throws this warning message if it finds an else statement and the line
> > +    above it is a break/continue/return statement indented at one tab more
> > +    than the else statement. So there can be some false positives like::
> > +
> > +      int n = 15;
> > +      if (n > 10)
> > +              n--;
> > +      else if (n == 10)
> > +              return 0;
> > +      else
> > +              n++;
> > +
> > +    Now the checkpatch will give a warning for the use of else after return
> > +    statement. If the else statement is removed then::
> > +
> > +      int n = 15;
> > +      if (n > 10)
> > +              n--;
> > +      else if (n == 10)
> > +              return 0;
> > +      n++;
> > +
> > +    Now both the n-- and n++ statements will be executed which is different
> > +    from the logic in the first case. As the if block doesn't have a return
> > +    statement, so removing the else statement is wrong.
> > +
> > +    Always check the previous if/else if blocks, for break/continue/return
> > +    statements, and do not blindly follow the checkpatch advice. One
> > +    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
> > +    even made it to the mainline, which was again reverted and fixed.
> > +    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else
>
> s/unnecesary/unnecessary
> > +    after return statement.")
> > +
> > +    Also, do not change the code if there is only a single return statement
> > +    inside if-else block, like::
> > +
> > +      if (a > b)
> > +              return a;
> > +      else
> > +              return b;
> > +
> > +    now if the else statement is removed::
> > +
> > +      if (a > b)
> > +              return a;
> > +      return b;
> > +
> > +    there is no considerable increase in the readability and one can argue
> > +    that the first form is more readable because of the indentation. So
> > +    do not remove the else statement in case of a single return statement
> > +    inside the if-else block.
> > +    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
> > --
> > 2.25.1
> >
>
> I think this message is unnecessarily long for a warning that's understandable
> at best without the verbose part. Try to shorten it up with only what's
> required for a user to understand why the warning is there.
>

Dwaipayan, I actually considered all this interesting information and
all valuable background information on this rule.

Now, I would like to see all this information in the checkpatch
documentation. Maybe here, the expectations for the --verbose option
and the checkpatch documentation are slightly different.
IMHO, the need for the checkpatch documentation beats the --verbose
option. If checkpatch users really ask for --verbose help on this
rule, they are already questioning the value of a rule that is already
quite understandable (as you said). So, then we should convince them
with all background information and known false positives we
encountered.

I vote for keeping all information; wordsmithing and writing more
precisely is certainly doable.

Lukas

Re: [PATCH v3] checkpatch: add check for continue statement in UNNECESSARY_ELSE

[Lukas Bulwahn]

On Sat, Oct 2, 2021 at 4:03 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> UNNECESSARY_ELSE only checks for the usage of else after a return or
> break. But the same logic is also true for continue statement.

Just a bit nicer wording and improving your English writing:

But the same logic applies for the continue statement.

>
> else used after a continue statement is unnecessary. So add a test
> for continue statement also.
s/else/An else branch/

s/for continue statement/for the continue statement/
s/also/, too/

Other than that, all good. Great patch.

>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> ---
>  scripts/checkpatch.pl | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c27d2312cfc3..0eee086d87fe 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4011,15 +4011,15 @@ sub process {
>
>  # check indentation of any line with a bare else
>  # (but not if it is a multiple line "if (foo) return bar; else return baz;")
> -# if the previous line is a break or return and is indented 1 tab more...
> +# if the previous line is a break or continue or return and is indented 1 tab more...
>                 if ($sline =~ /^\+([\t]+)(?:}[ \t]*)?else(?:[ \t]*{)?\s*$/) {
>                         my $tabs = length($1) + 1;
> -                       if ($prevline =~ /^\+\t{$tabs,$tabs}break\b/ ||
> -                           ($prevline =~ /^\+\t{$tabs,$tabs}return\b/ &&
> -                            defined $lines[$linenr] &&
> -                            $lines[$linenr] !~ /^[ \+]\t{$tabs,$tabs}return/)) {
> +                       if ($prevline =~ /^\+\t{$tabs,$tabs}(break|continue|return)\b/ &&
> +                           !($1 eq "return" &&
> +                             defined $lines[$linenr] &&
> +                             $lines[$linenr] =~ /^[ \+]\t{$tabs,$tabs}return/)) {
>                                 WARN("UNNECESSARY_ELSE",
> -                                    "else is not generally useful after a break or return\n" . $hereprev);
> +                                    "else is not generally useful after a $1\n" . $hereprev);
>                         }
>                 }
>
> --
> 2.25.1
>

Re: [PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

[Utkarsh Verma]

On Sun, Oct 03, 2021 at 10:08:17AM +0530, Dwaipayan Ray wrote:
> On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> >
> > Added and documented UNNECESSARY_ELSE message type.
> >
> > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > ---
> > Changes in v2:
> >   - Included the continue statement.
> >
> >  Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
> >  1 file changed, 77 insertions(+)
> >
> > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > index f0956e9ea2d8..b7c41e876d1d 100644
> > --- a/Documentation/dev-tools/checkpatch.rst
> > +++ b/Documentation/dev-tools/checkpatch.rst
> > @@ -1166,3 +1166,80 @@ Others
> >
> >    **TYPO_SPELLING**
> >      Some words may have been misspelled.  Consider reviewing them.
> > +
> > +  **UNNECESSARY_ELSE**
> > +    Using an else statement just after a return/break/continue statement is
> > +    unnecessary. 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);
> > +      }
> > +
> > +    It helps to reduce the indentation and removes the unnecessary else
> > +    statement. But note, there can be some false positives because of the
> > +    way it is implemented in the checkpatch script. The checkpatch script
> > +    throws this warning message if it finds an else statement and the line
> > +    above it is a break/continue/return statement indented at one tab more
> > +    than the else statement. So there can be some false positives like::
> > +
> > +      int n = 15;
> > +      if (n > 10)
> > +              n--;
> > +      else if (n == 10)
> > +              return 0;
> > +      else
> > +              n++;
> > +
> > +    Now the checkpatch will give a warning for the use of else after return
> > +    statement. If the else statement is removed then::
> > +
> > +      int n = 15;
> > +      if (n > 10)
> > +              n--;
> > +      else if (n == 10)
> > +              return 0;
> > +      n++;
> > +
> > +    Now both the n-- and n++ statements will be executed which is different
> > +    from the logic in the first case. As the if block doesn't have a return
> > +    statement, so removing the else statement is wrong.
> > +
> > +    Always check the previous if/else if blocks, for break/continue/return
> > +    statements, and do not blindly follow the checkpatch advice. One
> > +    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
> > +    even made it to the mainline, which was again reverted and fixed.
> > +    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else
> 
> s/unnecesary/unnecessary

It is a spelling mistake in the commit message itself, and I have quoted
that message, so I didn't change the message.

> > +    after return statement.")
> > +
> > +    Also, do not change the code if there is only a single return statement
> > +    inside if-else block, like::
> > +
> > +      if (a > b)
> > +              return a;
> > +      else
> > +              return b;
> > +
> > +    now if the else statement is removed::
> > +
> > +      if (a > b)
> > +              return a;
> > +      return b;
> > +
> > +    there is no considerable increase in the readability and one can argue
> > +    that the first form is more readable because of the indentation. So
> > +    do not remove the else statement in case of a single return statement
> > +    inside the if-else block.
> > +    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
> > --
> > 2.25.1
> >
> 
> I think this message is unnecessarily long for a warning that's understandable
> at best without the verbose part. Try to shorten it up with only what's
> required for a user to understand why the warning is there.
> 

Okay, I will try writing it more precisely as Lukas said.

> Dwaipayan.

Re: [PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

[Lukas Bulwahn]

also to: Julia Lawall for the idea on a cocci rule.

On Sat, Oct 2, 2021 at 4:45 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> Added and documented UNNECESSARY_ELSE message type.
>
> Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>

1. Julia, below is the description of a syntactic rule for unneeded
else branches; checkpatch implements a simple incomplete heuristics.
While reading this, I wondered if a student/learner for coccinelle
could actually write a cocci rule that does detect this pattern a bit
better than checkpatch. Maybe, you can add this to your (for
students'/newcomers') TODO list? It would certainly be interesting to
see some first evaluation.

2. Utkarsh, please send new patch versions v2, v3, etc. always as new
email threads, not as responses to the original patch.

See: https://lore.kernel.org/lkml/20211001120218.28751-1-utkarshverma294@gmail.com/T/#t

As you see, the email thread is very confusing; developers,
maintainers and tools have a hard time to find the latest version of
the patch and link the discussion. The process documentation should
already point out to NOT send next version patches as reply to older
patches.

Lukas

> ---
> Changes in v2:
>   - Included the continue statement.
>
>  Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>
> diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> index f0956e9ea2d8..b7c41e876d1d 100644
> --- a/Documentation/dev-tools/checkpatch.rst
> +++ b/Documentation/dev-tools/checkpatch.rst
> @@ -1166,3 +1166,80 @@ Others
>
>    **TYPO_SPELLING**
>      Some words may have been misspelled.  Consider reviewing them.
> +
> +  **UNNECESSARY_ELSE**
> +    Using an else statement just after a return/break/continue statement is
> +    unnecessary. 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);
> +      }
> +
> +    It helps to reduce the indentation and removes the unnecessary else
> +    statement. But note, there can be some false positives because of the
> +    way it is implemented in the checkpatch script. The checkpatch script
> +    throws this warning message if it finds an else statement and the line
> +    above it is a break/continue/return statement indented at one tab more
> +    than the else statement. So there can be some false positives like::
> +
> +      int n = 15;
> +      if (n > 10)
> +              n--;
> +      else if (n == 10)
> +              return 0;
> +      else
> +              n++;
> +
> +    Now the checkpatch will give a warning for the use of else after return
> +    statement. If the else statement is removed then::
> +
> +      int n = 15;
> +      if (n > 10)
> +              n--;
> +      else if (n == 10)
> +              return 0;
> +      n++;
> +
> +    Now both the n-- and n++ statements will be executed which is different
> +    from the logic in the first case. As the if block doesn't have a return
> +    statement, so removing the else statement is wrong.
> +
> +    Always check the previous if/else if blocks, for break/continue/return
> +    statements, and do not blindly follow the checkpatch advice. One
> +    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
> +    even made it to the mainline, which was again reverted and fixed.
> +    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else
> +    after return statement.")
> +
> +    Also, do not change the code if there is only a single return statement
> +    inside if-else block, like::
> +
> +      if (a > b)
> +              return a;
> +      else
> +              return b;
> +
> +    now if the else statement is removed::
> +
> +      if (a > b)
> +              return a;
> +      return b;
> +
> +    there is no considerable increase in the readability and one can argue
> +    that the first form is more readable because of the indentation. So
> +    do not remove the else statement in case of a single return statement
> +    inside the if-else block.
> +    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
> --
> 2.25.1
>

Re: [PATCH v2] docs: checkpatch: add UNNECESSARY_ELSE message

[Lukas Bulwahn]

On Sun, Oct 3, 2021 at 7:19 AM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
>
> On Sun, Oct 03, 2021 at 10:08:17AM +0530, Dwaipayan Ray wrote:
> > On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <utkarshverma294@gmail.com> wrote:
> > >
> > > Added and documented UNNECESSARY_ELSE message type.
> > >
> > > Signed-off-by: Utkarsh Verma <utkarshverma294@gmail.com>
> > > ---
> > > Changes in v2:
> > >   - Included the continue statement.
> > >
> > >  Documentation/dev-tools/checkpatch.rst | 77 ++++++++++++++++++++++++++
> > >  1 file changed, 77 insertions(+)
> > >
> > > diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
> > > index f0956e9ea2d8..b7c41e876d1d 100644
> > > --- a/Documentation/dev-tools/checkpatch.rst
> > > +++ b/Documentation/dev-tools/checkpatch.rst
> > > @@ -1166,3 +1166,80 @@ Others
> > >
> > >    **TYPO_SPELLING**
> > >      Some words may have been misspelled.  Consider reviewing them.
> > > +
> > > +  **UNNECESSARY_ELSE**
> > > +    Using an else statement just after a return/break/continue statement is
> > > +    unnecessary. 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);
> > > +      }
> > > +
> > > +    It helps to reduce the indentation and removes the unnecessary else
> > > +    statement. But note, there can be some false positives because of the
> > > +    way it is implemented in the checkpatch script. The checkpatch script
> > > +    throws this warning message if it finds an else statement and the line
> > > +    above it is a break/continue/return statement indented at one tab more
> > > +    than the else statement. So there can be some false positives like::
> > > +
> > > +      int n = 15;
> > > +      if (n > 10)
> > > +              n--;
> > > +      else if (n == 10)
> > > +              return 0;
> > > +      else
> > > +              n++;
> > > +
> > > +    Now the checkpatch will give a warning for the use of else after return
> > > +    statement. If the else statement is removed then::
> > > +
> > > +      int n = 15;
> > > +      if (n > 10)
> > > +              n--;
> > > +      else if (n == 10)
> > > +              return 0;
> > > +      n++;
> > > +
> > > +    Now both the n-- and n++ statements will be executed which is different
> > > +    from the logic in the first case. As the if block doesn't have a return
> > > +    statement, so removing the else statement is wrong.
> > > +
> > > +    Always check the previous if/else if blocks, for break/continue/return
> > > +    statements, and do not blindly follow the checkpatch advice. One
> > > +    patch (https://lore.kernel.org/all/20200615155131.GA4563@sevic69/)
> > > +    even made it to the mainline, which was again reverted and fixed.
> > > +    Commit 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else
> >
> > s/unnecesary/unnecessary
>
> It is a spelling mistake in the commit message itself, and I have quoted
> that message, so I didn't change the message.
>
> > > +    after return statement.")

I wonder if this detailed description of the example belongs here; and
we summarize it as:

Do not blindly follow checkpatch's advice here, as blind changes due
to this rule have already caused some disturbance, see commit ....

> > > +
> > > +    Also, do not change the code if there is only a single return statement
> > > +    inside if-else block, like::
> > > +
> > > +      if (a > b)
> > > +              return a;
> > > +      else
> > > +              return b;
> > > +
> > > +    now if the else statement is removed::
> > > +
> > > +      if (a > b)
> > > +              return a;
> > > +      return b;
> > > +
> > > +    there is no considerable increase in the readability and one can argue
> > > +    that the first form is more readable because of the indentation. So
> > > +    do not remove the else statement in case of a single return statement
> > > +    inside the if-else block.
> > > +    See: https://lore.kernel.org/lkml/20140925032215.GK7996@ZenIV.linux.org.uk/
> > > --
> > > 2.25.1
> > >
> >
> > I think this message is unnecessarily long for a warning that's understandable
> > at best without the verbose part. Try to shorten it up with only what's
> > required for a user to understand why the warning is there.
> >
>
> Okay, I will try writing it more precisely as Lukas said.
>
> > Dwaipayan.

[PATCH] Documentation: checkpatch: Document some more message types

[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

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

[Dwaipayan Ray]

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.

[PATCH] Documentation: checkpatch: Document some more message types

[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