[v4] coding-style: Clarify the expectations around bool

There has been some confusion since checkpatch started warning about bool
use in structures, and people have been avoiding using it.

Many people feel there is still a legitimate place for bool in structures,
so provide some guidance on bool usage derived from the entire thread that
spawned the checkpatch warning.

Link: https://lkml.kernel.org/r/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com
Signed-off-by: Joe Perches <joe@perches.com>
Acked-by: Joe Perches <joe@perches.com>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
 Documentation/process/coding-style.rst | 38 +++++++++++++++++++++++---
 scripts/checkpatch.pl                  | 13 ---------
 2 files changed, 34 insertions(+), 17 deletions(-)

v4:
- Describe true/false as definitions [Joe]
- Use clearer language for the _Bool explanation [Bart]
- Delete the checkpatch tests [Joe]

On Thu, Jan 10, 2019 at 11:48:13PM +0000, Jason Gunthorpe wrote:
> There has been some confusion since checkpatch started warning about bool
> use in structures, and people have been avoiding using it.
> 
> Many people feel there is still a legitimate place for bool in structures,
> so provide some guidance on bool usage derived from the entire thread that
> spawned the checkpatch warning.

Hey Jason,

I very much agree that the bool expectations could be much clearer, and this
patch is a nice step in that direction! Just a couple small nitpicks:

> +Do not use bool if cache line layout or size of the value matters, its size
> +and alignment varies based on the compiled architecture. Structures that are
> +optimized for alignment and size should not use bool.

+Do not use bool if cache line layout or size of the value matters, as its size
                                                                    ^
                                                                    |
Adding an "as" makes the sentence flow a bit cleaner: --------------

> +into a single bitwise 'flags' argument and 'flags' can often a more readable
> +alternative if the call-sites have naked true/false constants.

+into a single bitwise 'flags' argument and 'flags' can often be a more readable
                                                              ^
                                                              |
Missing a "be" here: -----------------------------------------

Ack from me after those two corrections.

Reviewed-by: Joey Pabalinas <joeypabalinas@gmail.com>

On 2019-01-11 00:48, Jason Gunthorpe wrote:
> There has been some confusion since checkpatch started warning about 
> bool
> use in structures, and people have been avoiding using it.
> 
> Many people feel there is still a legitimate place for bool in 
> structures,
> so provide some guidance on bool usage derived from the entire thread 
> that
> spawned the checkpatch warning.
> 
> Link:
> https://lkml.kernel.org/r/CA+55aFwVZk1OfB9T2v014PTAKFhtVan_Zj2dOjnCy3x6E4UJfA@mail.gmail.com
> Signed-off-by: Joe Perches <joe@perches.com>
> Acked-by: Joe Perches <joe@perches.com>
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
>  Documentation/process/coding-style.rst | 38 +++++++++++++++++++++++---
>  scripts/checkpatch.pl                  | 13 ---------
>  2 files changed, 34 insertions(+), 17 deletions(-)
> 
> v4:
> - Describe true/false as definitions [Joe]
> - Use clearer language for the _Bool explanation [Bart]
> - Delete the checkpatch tests [Joe]
> 
> diff --git a/Documentation/process/coding-style.rst
> b/Documentation/process/coding-style.rst
> index b78dd680c03809..db3e030d0df908 100644
> --- a/Documentation/process/coding-style.rst
> +++ b/Documentation/process/coding-style.rst
> @@ -921,7 +921,37 @@ result.  Typical examples would be functions that
> return pointers; they use
>  NULL or the ERR_PTR mechanism to report failure.
> 
> 
> -17) Don't re-invent the kernel macros
> +17) Using bool
> +--------------
> +
> +The Linux kernel bool type is an alias for the C99 _Bool type. bool 
> values can
> +only evaluate to 0 or 1, and implicit or explicit conversion to bool
> +automatically converts the value to true or false. When using bool 
> types the
> +!! construction is not needed, which eliminates a class of bugs.
> +
> +When working with bool values the true and false definitions should be 
> used
> +instead of 0 and 1.

A very minor thing. I would suggest to keep consistent, in the 
statement, the mapping
between definitions ("true and false [...]") and their correspondent 
integer values
("[...] instead of 1 and 0").

In few words, I propose to change "0 and 1" into "1 and 0".

> +
> +bool function return types and stack variables are always fine to use 
> whenever
> +appropriate. Use of bool is encouraged to improve readability and is 
> often a
> +better option than 'int' for storing boolean values.
> +
> +Do not use bool if cache line layout or size of the value matters, its 
> size
> +and alignment varies based on the compiled architecture. Structures 
> that are
> +optimized for alignment and size should not use bool.
> +
> +If a structure has many true/false values, consider consolidating them 
> into a
> +bitfield with 1 bit members, or using an appropriate fixed width type, 
> such as
> +u8.
> +
> +Similarly for function arguments, many true/false values can be 
> consolidated
> +into a single bitwise 'flags' argument and 'flags' can often a more 
> readable
> +alternative if the call-sites have naked true/false constants.

Of course, English is not my primary language, but it looks to me that 
here a "be"
is missing: "[...] and 'flags' can often a more readable alternative 
[...]".

> +
> +Otherwise limited use of bool in structures and arguments can improve
> +readability.

I'm going to update the Italian translations for this. Do you want me to 
contribute
directly to this patch? Otherwise I will send a dedicated patch later 
when this one
get accepted.

Thanks

> +18) Don't re-invent the kernel macros
>  -------------------------------------
> 
>  The header file include/linux/kernel.h contains a number of macros 
> that
> @@ -944,7 +974,7 @@ need them.  Feel free to peruse that header file
> to see what else is already
>  defined that you shouldn't reproduce in your code.
> 
> 
> -18) Editor modelines and other cruft
> +19) Editor modelines and other cruft
>  ------------------------------------
> 
>  Some editors can interpret configuration information embedded in 
> source files,
> @@ -978,7 +1008,7 @@ own custom mode, or may have some other magic
> method for making indentation
>  work correctly.
> 
> 
> -19) Inline assembly
> +20) Inline assembly
>  -------------------
> 
>  In architecture-specific code, you may need to use inline assembly to 
> interface
> @@ -1010,7 +1040,7 @@ the next instruction in the assembly output:
>  	     : /* outputs */ : /* inputs */ : /* clobbers */);
> 
> 
> -20) Conditional Compilation
> +21) Conditional Compilation
>  ---------------------------
> 
>  Wherever possible, don't use preprocessor conditionals (#if, #ifdef) 
> in .c
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index b737ca9d720441..d62abd032885a1 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6368,19 +6368,6 @@ sub process {
>  			}
>  		}
> 
> -# check for bool bitfields
> -		if ($sline =~ /^.\s+bool\s*$Ident\s*:\s*\d+\s*;/) {
> -			WARN("BOOL_BITFIELD",
> -			     "Avoid using bool as bitfield.  Prefer bool bitfields as
> unsigned int or u<8|16|32>\n" . $herecurr);
> -		}
> -
> -# check for bool use in .h files
> -		if ($realfile =~ /\.h$/ &&
> -		    $sline =~ /^.\s+bool\s*$Ident\s*(?::\s*d+\s*)?;/) {
> -			CHK("BOOL_MEMBER",
> -			    "Avoid using bool structure members because of possible
> alignment issues - see: https://lkml.org/lkml/2017/11/21/384\n" .
> $herecurr);
> -		}
> -
>  # check for semaphores initialized locked
>  		if ($line =~ /^.\s*sema_init.+,\W?0\W?\)/) {
>  			WARN("CONSIDER_COMPLETION",

On Thu, Jan 10, 2019 at 11:48:13PM +0000, Jason Gunthorpe wrote:
> +The Linux kernel bool type is an alias for the C99 _Bool type. bool values can
> +only evaluate to 0 or 1, and implicit or explicit conversion to bool
> +automatically converts the value to true or false. When using bool types the
> +!! construction is not needed, which eliminates a class of bugs.
> +
> +When working with bool values the true and false definitions should be used
> +instead of 0 and 1.
> +
> +bool function return types and stack variables are always fine to use whenever
> +appropriate. Use of bool is encouraged to improve readability and is often a
> +better option than 'int' for storing boolean values.

It's awkward to start a sentence with a lower case letter.  How about
rephrasing this paragraph and the following one as:

  Using bool as the return type of a function or as a variable is always
  fine when appropriate.  It often improves readability and is a better option
  than int for storing boolean values.  Using bool in data structures is
  more debatable; its size and alignment can vary between architectures.

> +Do not use bool if cache line layout or size of the value matters, its size
> +and alignment varies based on the compiled architecture. Structures that are
> +optimized for alignment and size should not use bool.
> +
> +If a structure has many true/false values, consider consolidating them into a
> +bitfield with 1 bit members, or using an appropriate fixed width type, such as
> +u8.

On Fri, Jan 11, 2019 at 07:29:40AM -1000, Joey Pabalinas wrote:
> On Thu, Jan 10, 2019 at 11:48:13PM +0000, Jason Gunthorpe wrote:
> > There has been some confusion since checkpatch started warning about bool
> > use in structures, and people have been avoiding using it.
> > 
> > Many people feel there is still a legitimate place for bool in structures,
> > so provide some guidance on bool usage derived from the entire thread that
> > spawned the checkpatch warning.
> 
> Hey Jason,
> 
> I very much agree that the bool expectations could be much clearer, and this
> patch is a nice step in that direction! Just a couple small nitpicks:
> 
> > +Do not use bool if cache line layout or size of the value matters, its size
> > +and alignment varies based on the compiled architecture. Structures that are
> > +optimized for alignment and size should not use bool.
> 
> +Do not use bool if cache line layout or size of the value matters, as its size
>                                                                     ^
>                                                                     |
> Adding an "as" makes the sentence flow a bit cleaner: --------------
> 
> > +into a single bitwise 'flags' argument and 'flags' can often a more readable
> > +alternative if the call-sites have naked true/false constants.
> 
> +into a single bitwise 'flags' argument and 'flags' can often be a more readable
>                                                               ^
>                                                               |
> Missing a "be" here: -----------------------------------------
> 
> Ack from me after those two corrections.
> 
> Reviewed-by: Joey Pabalinas <joeypabalinas@gmail.com>

done, thanks

Jason

On Sun, Jan 13, 2019 at 08:49:36AM -0800, Matthew Wilcox wrote:
> On Thu, Jan 10, 2019 at 11:48:13PM +0000, Jason Gunthorpe wrote:
> > +The Linux kernel bool type is an alias for the C99 _Bool type. bool values can
> > +only evaluate to 0 or 1, and implicit or explicit conversion to bool
> > +automatically converts the value to true or false. When using bool types the
> > +!! construction is not needed, which eliminates a class of bugs.
> > +
> > +When working with bool values the true and false definitions should be used
> > +instead of 0 and 1.
> > +
> > +bool function return types and stack variables are always fine to use whenever
> > +appropriate. Use of bool is encouraged to improve readability and is often a
> > +better option than 'int' for storing boolean values.
> 
> It's awkward to start a sentence with a lower case letter.  How about
> rephrasing this paragraph and the following one as:
> 
>   Using bool as the return type of a function or as a variable is always
>   fine when appropriate.  It often improves readability and is a better option
>   than int for storing boolean values.  Using bool in data structures is
>   more debatable; its size and alignment can vary between architectures.

This is more concise, but I think if the coding style is not going to
give a concrete advise then it should at least provide some general
information so the reader can try and make an informed choice.

That is why I had it expand on some of the rationals a little bit,
along with a concrete direction to not use bool in the cases Linus
specifically called out.

> > +Do not use bool if cache line layout or size of the value matters, its size
> > +and alignment varies based on the compiled architecture. Structures that are
> > +optimized for alignment and size should not use bool.
> > +
> > +If a structure has many true/false values, consider consolidating them into a
> > +bitfield with 1 bit members, or using an appropriate fixed width type, such as
> > +u8.

JAson

On Sun, Jan 13, 2019 at 05:01:39PM +0100, Federico Vaga wrote:

> > -17) Don't re-invent the kernel macros
> > +17) Using bool
> > +--------------
> > +
> > +The Linux kernel bool type is an alias for the C99 _Bool type. bool
> > values can
> > +only evaluate to 0 or 1, and implicit or explicit conversion to bool
> > +automatically converts the value to true or false. When using bool
> > types the
> > +!! construction is not needed, which eliminates a class of bugs.
> > +
> > +When working with bool values the true and false definitions should be
> > used
> > +instead of 0 and 1.
> 
> A very minor thing. I would suggest to keep consistent, in the
> statement, the mapping between definitions ("true and false [...]")
> and their correspondent integer values ("[...] instead of 1 and 0").
> 
> In few words, I propose to change "0 and 1" into "1 and 0".

Hm, sure, seems harmless

> > +Similarly for function arguments, many true/false values can be
> > consolidated
> > +into a single bitwise 'flags' argument and 'flags' can often a more
> > readable
> > +alternative if the call-sites have naked true/false constants.
> 
> Of course, English is not my primary language, but it looks to me
> that here a "be" is missing: "[...] and 'flags' can often a more
> readable alternative [...]".

yes, sthanks
 
> > +Otherwise limited use of bool in structures and arguments can improve
> > +readability.
> 
> I'm going to update the Italian translations for this. Do you want
> me to contribute directly to this patch? Otherwise I will send a
> dedicated patch later when this one get accepted.
 
I think you should send it as an update I guess? I don't really know
the process for translations

Jason