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

[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

[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 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]

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.

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] 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

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] 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
>