2022-08-02 06:12:07

by Utkarsh Verma

[permalink] [raw]
Subject: [PATCH] docs: checkpatch: add some new checkpatch documentation messages

Added and documented the following new message types:
- JIFFIES_COMPARISON
- LONG_UDELAY
- MSLEEP
- INDENTED_LABEL
- IF_0
- IF_1
- MISORDERED_TYPE
- UNNECESSARY_BREAK
- UNNECESSARY_ELSE
- UNNECESSARY_INT
- UNSPECIFIED_INT

Signed-off-by: Utkarsh Verma <[email protected]>
---
Documentation/dev-tools/checkpatch.rst | 300 +++++++++++++++++++++++++
1 file changed, 300 insertions(+)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index b52452bc2963..78abcadb5228 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -445,6 +445,34 @@ API usage

See: https://lore.kernel.org/lkml/[email protected]/

+ **JIFFIES_COMPARISON**
+ Doing time comparison by directly using relational operators with jiffies
+ or get_jiffies_64() is wrong. Example::
+
+ if (jiffies > timeout)
+ do_something;
+
+ The above code is wrong because when the jiffies value reaches its maximum
+ limit, then it overflows and wraps around zero, so the above code will
+ not work as intended. To correctly handle this jiffies overflow condition,
+ the kernel provides some helper macros defined in <linux/jiffies.h>.
+ Some of the important macros are::
+
+ int time_after(unsigned long a, unsigned long b);
+ int time_before(unsigned long a, unsigned long b);
+ int time_after_eq(unsigned long a, unsigned long b);
+ int time_before_eq(unsigned long a, unsigned long b);
+
+ So the above code can be corrected by::
+
+ if (time_after(jiffies, timeout))
+ do_something;
+
+ Also, by using these macros, the code is easier to maintain and
+ future-proof because if the timer wrap changes in the future, then there
+ will be no need to alter the driver code. So it is strongly recommended
+ to always use these macros instead of direct comparison with jiffies.
+
**LOCKDEP**
The lockdep_no_validate class was added as a temporary measure to
prevent warnings on conversion of device->sem to device->mutex.
@@ -452,11 +480,68 @@ API usage

See: https://lore.kernel.org/lkml/1268959062.9440.467.camel@laptop/

+ **LONG_UDELAY**
+ Passing large arguments to udelay() results in integer overflow in the
+ internal loop calculation of udelay() and the driver code will display
+ an error of unresolved symbol, __bad_udelay.
+
+ Different CPU architectures have different threshold values for udelay()
+ after which they call __bad_udelay(). For ARM architecture, the threshold
+ value is 2000us. For the exact maths behind this limit see
+ arch/arm/include/asm/delay.h
+
+ This checkpatch warning is triggered when the argument passed to udelay()
+ is greater than 2000us. Though for some architectures this threshold
+ value is more than 2000us, but as a general rule if the wanted delay is
+ in thousand of micro seconds, then use mdelay().
+
+ mdelay() is a wrapper around udelay() that accounts for the overflow
+ condition when passing large arguments to udelay(). Also, note that the
+ udelay() and mdelay() are busy-waiting, other tasks cannot run during
+ that time. So if the hardware supports hrtimers, then a better approach
+ would be to use usleep_range() function. For delaying 10us - 20ms,
+ usleep_range() is the recommended API. But make a change to usleep_range()
+ only if the hardware supports hrtimers, along with proper testing on a
+ real hardware.
+
+ If the delay is more than 20ms then use msleep(). msleep() is the
+ recommended API for delaying more than 20ms because it is implemented
+ using the jiffies timer and does not involve busy-waiting.
+
+ See: https://www.kernel.org/doc/html/latest/timers/timers-howto.html
+
**MALFORMED_INCLUDE**
The #include statement has a malformed path. This has happened
because the author has included a double slash "//" in the pathname
accidentally.

+ **MSLEEP**
+ msleep() is implemented using the jiffies counter. When msleep() is
+ called it stops for a minimum of 2 jiffies. So, depending on the value
+ of HZ the mleep will atleast stop for 1-20ms. In worst case, if HZ=100
+ then msleep() will delay for a minimum of 20ms
+ (https://lore.kernel.org/all/[email protected]/). So for
+ sleeping for 10us-20ms it is recommended to use usleep_range() and not
+ msleep().
+
+ But ignore this warning, if the change to usleep_range() cannot be tested
+ on a real hardware.
+
+ usleep_range() is implemented using the hrtimer. Some hardware doesn't
+ support hrtimer, so no sense in making the change to usleep_range().
+
+ Also, the min and max value in usleep_range(unsigned long min, unsigned
+ long max) must be selected by understanding the driver code, and the
+ hardware with lots of testing and verification. One can see these timing
+ changes that others have made to get some approximate min and max values,
+ and then test to get the best possible values.
+
+ See:
+
+ 1. https://lore.kernel.org/all/alpine.DEB.2.22.394.2110171140040.3026@hadrien/
+ 2. https://lore.kernel.org/linux-staging/[email protected]/
+ 3. https://lore.kernel.org/all/[email protected]/
+
**USE_LOCKDEP**
lockdep_assert_held() annotations should be preferred over
assertions based on spin_is_locked()
@@ -661,6 +746,30 @@ 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 (https://lore.kernel.org/all/[email protected]/).
+ The Linux Kernel Coding Style does not indent the goto labels. Since
+ the program control can directly jump from one part of the program to a
+ pre-defined goto label, so making these labels easy to spot is important.
+ By flushing the goto labels to the left, these labels are more visible
+ and easier to identify.
+
+ Since the checkpatch only uses the regular expressions for finding the
+ possible coding style violation in a patch, and does not track the logic
+ or flow of the program, so there can be some false positives. Though for
+ this rule it is rare, still do not blindly follow the checkpatch advice.
+
+ Suppose if there is a conditional (ternary) operator across multiple
+ lines and there is no space around the “:” then this warning can be
+ triggered. Example::
+
+ return_value = (function1(value1) && function2(value2)) ?
+ NULL: some_other_value;
+
+ Here the checkpatch will give this label warning along with the spacing
+ warning.
+
**SWITCH_CASE_INDENT_LEVEL**
switch should be at the same indent as case.
Example::
@@ -823,6 +932,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.
@@ -1231,6 +1353,49 @@ Others
The memset use appears to be incorrect. This may be caused due to
badly ordered parameters. Please recheck the usage.

+ **MISORDERED_TYPE**
+ According to section “6.7.2 Type Specifiers” in C90 standard, “type
+ specifiers may occur in any order.” This means that "signed long long
+ int" is same as "long long int signed". But to avoid confusion and make
+ the code easier to read, the declaration type should use the following
+ format::
+
+ [[un]signed] [short|int|long|long long]
+
+ Below is the list of standard integer types. Each row lists all the
+ different ways of specifying a particular type delimited by commas.
+ Note: Also include all the permutations of a particular type
+ on the left column delimited by comma. For example, the permutations
+ for "signed long int" are "signed int long", "long signed int",
+ "long int signed", "int signed long", and "int long signed".
+
+ +--------------------------------------------------+--------------------+
+ | Types | Recommended Way |
+ +=======================================================================+
+ | char | char |
+ +-----------------------------------------------------------------------+
+ | signed char | signed char |
+ +-----------------------------------------------------------------------+
+ | unsigned char | unsigned char |
+ +-----------------------------------------------------------------------+
+ | signed, int, signed int | int |
+ +-----------------------------------------------------------------------+
+ | unsigned, unsigned int | unsigned int |
+ +-----------------------------------------------------------------------+
+ | short, signed short, short int, signed short int | short |
+ +-----------------------------------------------------------------------+
+ | unsigned short, unsigned short int | unsigned short |
+ +-----------------------------------------------------------------------+
+ | long, signed long, long int, signed long int | long |
+ +-----------------------------------------------------------------------+
+ | unsigned long, unsigned long int | unsigned long |
+ +-----------------------------------------------------------------------|
+ | long long, signed long long, long long int, | long long |
+ | signed long long int | |
+ +-----------------------------------------------------------------------+
+ | unsigned long long, unsigned long long int | unsigned long long |
+ +-----------------------------------------------------------------------+
+
**NOT_UNIFIED_DIFF**
The patch file does not appear to be in unified-diff format. Please
regenerate the patch file before sending it to the maintainer.
@@ -1247,3 +1412,138 @@ Others

**TYPO_SPELLING**
Some words may have been misspelled. Consider reviewing them.
+
+ **UNNECESSARY_BREAK**
+ Using break statement just after a goto, return or break is unnecessary.
+ For example::
+
+ switch (foo) {
+ case 1:
+ goto err;
+ break;
+ }
+
+ Here, the break statement is completely unnecessary, because it will
+ never be executed. So it is better to remove it.
+
+ It is not a bug or syntactically incorrect, but it should be removed
+ because it is an unreachable statement that will never be executed
+ (https://lore.kernel.org/lkml/[email protected]/).
+
+ Note there can be some false positives, which happen because of the way
+ this rule is implemented in the checkpatch script. The checkpatch script
+ throws this warning message if it finds a break statement and the line
+ above it is a goto/return/break statement with the same indentation
+ (same number of tabs). It only relies on the same indentation and does
+ not care about the logic of the code. For example::
+
+ if (foo)
+ break;
+ break;
+
+ Here the checkpatch will throw this warning message, because both the
+ break statements are at the same indentation. The second break statement
+ is unnecessarily indented, which causes this false positive. So do not
+ blindly follow the checkpatch advice here, instead consider the logic of
+ the code before making the changes. In the above example, correct the
+ indentation of the second break statement instead of following the
+ checkpatch advice.
+
+ **UNNECESSARY_ELSE**
+ Using an else statement just after a return/break/continue statement is
+ unnecessary. For example::
+
+ if (foo)
+ break;
+ else
+ usleep(1);
+
+ is generally better written as::
+
+ if (foo)
+ break;
+ usleep(1);
+
+ It helps to reduce the indentation and removes the unnecessary else
+ statement. But do not blindly follow checkpatch's advice here, as blind
+ changes due to this rule have already caused some disturbance, see commit
+ 98fe05e21a6e ("staging: rtl8712: Remove unnecesary else after return
+ statement."). That commit made it to the mainline which had to be
+ reverted and fixed.
+
+ These false positives happen because of the way this rule 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. So always check the
+ previous if/else if blocks, for break/continue/return statements, before
+ following this rule.
+
+ 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]/
+
+ **UNNECESSARY_INT**
+ On Sun, 2018-08-05 at 08:52 -0700, Linus Torvalds wrote:
+ > "long unsigned int" isn't _technically_ wrong. But we normally
+ > call that type "unsigned long".
+
+ Using "int" type-specifier with "short", "long", and "long long" is
+ optional. So "short int" is same as "short", "long int" is same as
+ "long", and "long long int" is same as "long long". Similary for
+ their unsigned counterparts also.
+
+ To avoid confusion so that people do not interpret these synonymous
+ types as different types, and also to make the code uniform, clean and
+ more readable usage of "int" should be avoided with "short", "long",
+ and "long long".
+
+ See: https://lore.kernel.org/all/[email protected]/T/#m1fa34198ce2bd088b3520b74326468a2ab314ce7
+
+ **UNSPECIFIED_INT**
+ "signed", "signed int", and "int" are all the same types. Similarly,
+ "unsigned" and "unsigned int" are also the same.
+ So using type-specifier "signed" and "unsigned" with or without "int"
+ is the same type only. But to avoid confusion so that people do not
+ interpret these synonymous types as different types, and also to make
+ the code uniform, clean, and more readable prefer using the "signed int"
+ or "int" in place of "signed" and "unsigned int" in place of "unsigned".
--
2.25.1



2022-08-03 01:06:00

by kernel test robot

[permalink] [raw]
Subject: Re: [PATCH] docs: checkpatch: add some new checkpatch documentation messages

Hi Utkarsh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on lwn/docs-next]
[also build test ERROR on linus/master v5.19 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Utkarsh-Verma/docs-checkpatch-add-some-new-checkpatch-documentation-messages/20220802-135802
base: git://git.lwn.net/linux.git docs-next
reproduce: make htmldocs

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>

All error/warnings (new ones prefixed by >>):

>> Documentation/dev-tools/checkpatch.rst:1393: (SEVERE/4) Unexpected section title or transition.
>> Documentation/dev-tools/checkpatch.rst:1393: WARNING: Block quote ends without a blank line; unexpected unindent.
>> Documentation/dev-tools/checkpatch.rst:1393: WARNING: Unexpected section title or transition.

vim +1393 Documentation/dev-tools/checkpatch.rst

1302
1303 **CONFIG_DESCRIPTION**
1304 Kconfig symbols should have a help text which fully describes
1305 it.
1306
1307 **CORRUPTED_PATCH**
1308 The patch seems to be corrupted or lines are wrapped.
1309 Please regenerate the patch file before sending it to the maintainer.
1310
1311 **CVS_KEYWORD**
1312 Since linux moved to git, the CVS markers are no longer used.
1313 So, CVS style keywords ($Id$, $Revision$, $Log$) should not be
1314 added.
1315
1316 **DEFAULT_NO_BREAK**
1317 switch default case is sometimes written as "default:;". This can
1318 cause new cases added below default to be defective.
1319
1320 A "break;" should be added after empty default statement to avoid
1321 unwanted fallthrough.
1322
1323 **DOS_LINE_ENDINGS**
1324 For DOS-formatted patches, there are extra ^M symbols at the end of
1325 the line. These should be removed.
1326
1327 **DT_SCHEMA_BINDING_PATCH**
1328 DT bindings moved to a json-schema based format instead of
1329 freeform text.
1330
1331 See: https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-schema.html
1332
1333 **DT_SPLIT_BINDING_PATCH**
1334 Devicetree bindings should be their own patch. This is because
1335 bindings are logically independent from a driver implementation,
1336 they have a different maintainer (even though they often
1337 are applied via the same tree), and it makes for a cleaner history in the
1338 DT only tree created with git-filter-branch.
1339
1340 See: https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
1341
1342 **EMBEDDED_FILENAME**
1343 Embedding the complete filename path inside the file isn't particularly
1344 useful as often the path is moved around and becomes incorrect.
1345
1346 **FILE_PATH_CHANGES**
1347 Whenever files are added, moved, or deleted, the MAINTAINERS file
1348 patterns can be out of sync or outdated.
1349
1350 So MAINTAINERS might need updating in these cases.
1351
1352 **MEMSET**
1353 The memset use appears to be incorrect. This may be caused due to
1354 badly ordered parameters. Please recheck the usage.
1355
1356 **MISORDERED_TYPE**
1357 According to section “6.7.2 Type Specifiers” in C90 standard, “type
1358 specifiers may occur in any order.” This means that "signed long long
1359 int" is same as "long long int signed". But to avoid confusion and make
1360 the code easier to read, the declaration type should use the following
1361 format::
1362
1363 [[un]signed] [short|int|long|long long]
1364
1365 Below is the list of standard integer types. Each row lists all the
1366 different ways of specifying a particular type delimited by commas.
1367 Note: Also include all the permutations of a particular type
1368 on the left column delimited by comma. For example, the permutations
1369 for "signed long int" are "signed int long", "long signed int",
1370 "long int signed", "int signed long", and "int long signed".
1371
1372 +--------------------------------------------------+--------------------+
1373 | Types | Recommended Way |
1374 +=======================================================================+
1375 | char | char |
1376 +-----------------------------------------------------------------------+
1377 | signed char | signed char |
1378 +-----------------------------------------------------------------------+
1379 | unsigned char | unsigned char |
1380 +-----------------------------------------------------------------------+
1381 | signed, int, signed int | int |
1382 +-----------------------------------------------------------------------+
1383 | unsigned, unsigned int | unsigned int |
1384 +-----------------------------------------------------------------------+
1385 | short, signed short, short int, signed short int | short |
1386 +-----------------------------------------------------------------------+
1387 | unsigned short, unsigned short int | unsigned short |
1388 +-----------------------------------------------------------------------+
1389 | long, signed long, long int, signed long int | long |
1390 +-----------------------------------------------------------------------+
1391 | unsigned long, unsigned long int | unsigned long |
1392 +-----------------------------------------------------------------------|
> 1393 | long long, signed long long, long long int, | long long |

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-08-03 03:19:08

by Bagas Sanjaya

[permalink] [raw]
Subject: Re: [PATCH] docs: checkpatch: add some new checkpatch documentation messages

Hi Utkarsh,

On Tue, Aug 02, 2022 at 11:25:28AM +0530, Utkarsh Verma wrote:
> Added and documented the following new message types:
> - JIFFIES_COMPARISON
> - LONG_UDELAY
> - MSLEEP
> - INDENTED_LABEL
> - IF_0
> - IF_1
> - MISORDERED_TYPE
> - UNNECESSARY_BREAK
> - UNNECESSARY_ELSE
> - UNNECESSARY_INT
> - UNSPECIFIED_INT
>

Use imperative mood for patch description instead.

> + **MISORDERED_TYPE**
> + According to section “6.7.2 Type Specifiers” in C90 standard, “type
> + specifiers may occur in any order.” This means that "signed long long
> + int" is same as "long long int signed". But to avoid confusion and make
> + the code easier to read, the declaration type should use the following
> + format::
> +
> + [[un]signed] [short|int|long|long long]
> +
> + Below is the list of standard integer types. Each row lists all the
> + different ways of specifying a particular type delimited by commas.
> + Note: Also include all the permutations of a particular type
> + on the left column delimited by comma. For example, the permutations
> + for "signed long int" are "signed int long", "long signed int",
> + "long int signed", "int signed long", and "int long signed".
> +
> + +--------------------------------------------------+--------------------+
> + | Types | Recommended Way |
> + +=======================================================================+
> + | char | char |
> + +-----------------------------------------------------------------------+
> + | signed char | signed char |
> + +-----------------------------------------------------------------------+
> + | unsigned char | unsigned char |
> + +-----------------------------------------------------------------------+
> + | signed, int, signed int | int |
> + +-----------------------------------------------------------------------+
> + | unsigned, unsigned int | unsigned int |
> + +-----------------------------------------------------------------------+
> + | short, signed short, short int, signed short int | short |
> + +-----------------------------------------------------------------------+
> + | unsigned short, unsigned short int | unsigned short |
> + +-----------------------------------------------------------------------+
> + | long, signed long, long int, signed long int | long |
> + +-----------------------------------------------------------------------+
> + | unsigned long, unsigned long int | unsigned long |
> + +-----------------------------------------------------------------------|
> + | long long, signed long long, long long int, | long long |
> + | signed long long int | |
> + +-----------------------------------------------------------------------+
> + | unsigned long long, unsigned long long int | unsigned long long |
> + +-----------------------------------------------------------------------+
> +

The table above triggers htmldocs error, so I have to apply the fixup:

---- >8 ----

From 4fcc0df4ffcf1190330c12db5352cae03f8620fb Mon Sep 17 00:00:00 2001
From: Bagas Sanjaya <[email protected]>
Date: Wed, 3 Aug 2022 09:48:43 +0700
Subject: [PATCH] Documentation: checkpatch: MISORDERED_TYPE table intersection
fixup

Sphinx reported error and warnings pointed at MISORDERED_TYPE table:

Documentation/dev-tools/checkpatch.rst:1393: (SEVERE/4) Unexpected section title or transition.
Documentation/dev-tools/checkpatch.rst:1393: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/dev-tools/checkpatch.rst:1393: WARNING: Unexpected section title or transition.

Fix these above by marking cell intersections with plus (+) sign.

Link: https://lore.kernel.org/linux-doc/[email protected]/
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Bagas Sanjaya <[email protected]>
---
Documentation/dev-tools/checkpatch.rst | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/Documentation/dev-tools/checkpatch.rst b/Documentation/dev-tools/checkpatch.rst
index 78abcadb522824..a9d27913e6c46f 100644
--- a/Documentation/dev-tools/checkpatch.rst
+++ b/Documentation/dev-tools/checkpatch.rst
@@ -1371,30 +1371,30 @@ Others

+--------------------------------------------------+--------------------+
| Types | Recommended Way |
- +=======================================================================+
+ +==================================================+====================+
| char | char |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| signed char | signed char |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| unsigned char | unsigned char |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| signed, int, signed int | int |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| unsigned, unsigned int | unsigned int |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| short, signed short, short int, signed short int | short |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| unsigned short, unsigned short int | unsigned short |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| long, signed long, long int, signed long int | long |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| unsigned long, unsigned long int | unsigned long |
- +-----------------------------------------------------------------------|
+ +--------------------------------------------------+--------------------+
| long long, signed long long, long long int, | long long |
| signed long long int | |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+
| unsigned long long, unsigned long long int | unsigned long long |
- +-----------------------------------------------------------------------+
+ +--------------------------------------------------+--------------------+

**NOT_UNIFIED_DIFF**
The patch file does not appear to be in unified-diff format. Please
---- >8 ----

> + 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.
> +

So the first indentation above is more readable for it's clear that
b is returned if the condition is false (in this case, a < b), right?

> + See: https://lore.kernel.org/lkml/[email protected]/
> +
> + **UNNECESSARY_INT**
> + On Sun, 2018-08-05 at 08:52 -0700, Linus Torvalds wrote:
> + > "long unsigned int" isn't _technically_ wrong. But we normally
> + > call that type "unsigned long".
> +

IMO, the mail quote above can be deleted.

Thanks.

--
An old man doll... just what I always wanted! - Clara