Added and documented 3 new message types:
- UNNECESSARY_INT
- UNSPECIFIED_INT
- UNNECESSARY_ELSE
Signed-off-by: Utkarsh Verma <[email protected]>
---
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
Utkarsh Verma <[email protected]> writes:
> Added and documented 3 new message types:
> - UNNECESSARY_INT
> - UNSPECIFIED_INT
> - UNNECESSARY_ELSE
>
> Signed-off-by: Utkarsh Verma <[email protected]>
> ---
> 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
On Mon, Sep 27, 2021 at 11:43:59AM -0600, Jonathan Corbet wrote:
> Utkarsh Verma <[email protected]> writes:
>
> > Added and documented 3 new message types:
> > - UNNECESSARY_INT
> > - UNSPECIFIED_INT
> > - UNNECESSARY_ELSE
> >
> > Signed-off-by: Utkarsh Verma <[email protected]>
> > ---
> > 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
On Mon, 2021-09-27 at 11:43 -0600, Jonathan Corbet wrote:
> Utkarsh Verma <[email protected]> writes:
>
> > Added and documented 3 new message types:
> > - UNNECESSARY_INT
> > - UNSPECIFIED_INT
> > - UNNECESSARY_ELSE
> >
> > Signed-off-by: Utkarsh Verma <[email protected]>
> > ---
> > ?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 {
...
}
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
<[email protected]> wrote:
>
> Added and documented 3 new message types:
> - UNNECESSARY_INT
> - UNSPECIFIED_INT
> - UNNECESSARY_ELSE
>
> Signed-off-by: Utkarsh Verma <[email protected]>
> ---
> 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
>
Added and documented UNNECESSARY_ELSE message type.
Signed-off-by: Utkarsh Verma <[email protected]>
---
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/[email protected]/
--
2.25.1
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 <[email protected]> writes:
> >
> > > Added and documented 3 new message types:
> > > - UNNECESSARY_INT
> > > - UNSPECIFIED_INT
> > > - UNNECESSARY_ELSE
> > >
> > > Signed-off-by: Utkarsh Verma <[email protected]>
> > > ---
> > > ?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
Added and documented UNNECESSARY_ELSE message type.
Signed-off-by: Utkarsh Verma <[email protected]>
---
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/[email protected]/
--
2.25.1
On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <[email protected]> wrote:
>
> Added and documented UNNECESSARY_ELSE message type.
>
> Signed-off-by: Utkarsh Verma <[email protected]>
> ---
> 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/[email protected]/
> --
> 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.
On Sun, Oct 3, 2021 at 6:38 AM Dwaipayan Ray <[email protected]> wrote:
>
> On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <[email protected]> wrote:
> >
> > Added and documented UNNECESSARY_ELSE message type.
> >
> > Signed-off-by: Utkarsh Verma <[email protected]>
> > ---
> > 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/[email protected]/
> > --
> > 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
On Sun, Oct 03, 2021 at 10:08:17AM +0530, Dwaipayan Ray wrote:
> On Sat, Oct 2, 2021 at 8:15 PM Utkarsh Verma <[email protected]> wrote:
> >
> > Added and documented UNNECESSARY_ELSE message type.
> >
> > Signed-off-by: Utkarsh Verma <[email protected]>
> > ---
> > 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/[email protected]/
> > --
> > 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.
also to: Julia Lawall for the idea on a cocci rule.
On Sat, Oct 2, 2021 at 4:45 PM Utkarsh Verma <[email protected]> wrote:
>
> Added and documented UNNECESSARY_ELSE message type.
>
> Signed-off-by: Utkarsh Verma <[email protected]>
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/[email protected]/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/[email protected]/
> --
> 2.25.1
>
On Sun, Oct 3, 2021 at 7:19 AM Utkarsh Verma <[email protected]> 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 <[email protected]> wrote:
> > >
> > > Added and documented UNNECESSARY_ELSE message type.
> > >
> > > Signed-off-by: Utkarsh Verma <[email protected]>
> > > ---
> > > 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/[email protected]/
> > > --
> > > 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.