Re: [PATCH 0/4] int to bool conversion

Re: [PATCH 0/4] int to bool conversion

[Louis Langholtz]

While it may not be productive to perturb seemingly working
code (as Rafael argues), it may also not be productive to
have decreased code readability (as Quentin suggests).

Personally I prefer readability enhancements over worrying
about possibly breaking working code. I don't want to start
a flame war so I won't go into arguing this as a better
position. I'd just like to thank Quentin for his efforts to
identify boolean uses of variables. It's something I'm
interested in as well and have been working on in a branch
of my own git repository.

Quentin if you want to work on this together at all, that'd
be great. Please contact me directly as I'm not subscribed to
the LKML. As for the original semantic patch code, it's
unlikely that it would be safe to not exclude variables that
are passed by address (and seemingly the ampersand operator
applied on x - as in '&x' - should be a part of the exclusion
set).

Lou

Re: [PATCH 0/4] int to bool conversion

[Peter Hurley]

Hi Louis,

On 01/30/2015 12:32 PM, Louis Langholtz wrote:
> While it may not be productive to perturb seemingly working
> code (as Rafael argues), it may also not be productive to
> have decreased code readability (as Quentin suggests).
> 
> Personally I prefer readability enhancements over worrying
> about possibly breaking working code. I don't want to start
> a flame war so I won't go into arguing this as a better
> position. I'd just like to thank Quentin for his efforts to
> identify boolean uses of variables. It's something I'm
> interested in as well and have been working on in a branch
> of my own git repository.
> 
> Quentin if you want to work on this together at all, that'd
> be great. Please contact me directly as I'm not subscribed to
> the LKML. As for the original semantic patch code, it's
> unlikely that it would be safe to not exclude variables that
> are passed by address (and seemingly the ampersand operator
> applied on x - as in '&x' - should be a part of the exclusion
> set).

Just a quick note about bools vs. ints in kernel code:
one of the required arch guarantees is that an int is a
unique memory location, whereas a bool does not provide that
guarantee. Much kernel code requires unique memory locations.

For instance, in the example below, do_something() may not execute.

static bool x;
static bool y;

CPU 0                     | CPU 1
                          |
                          | y = 1;
x = 1;                    |
                          | if (y)
                          |    do_something();
                          |

The reason is that the 'x = 1' statement may be a RMW operation
if the compiler has merged x and y into the same memory
location. So that what really happens is

u8 xy;

CPU 0                     | CPU 1
                          |
                          | load [xy]=> R
			  | R |= Y_BIT
load [xy] => R            |
R |= X_BIT                |
                          | store R => [xy]
store R => [xy]           |
                          | if ([xy] & Y_BIT)
                          |    do_something();
                          |

I looked over the patches when they were first posted and
none involve concurrent access, so I didn't mention it.
But a general campaign of int=>bool will need to be aware
of this.

Regards,
Peter Hurley

PS - In fact, even chars or shorts can be RMW on the entire
machine word.

RE: [PATCH 0/4] int to bool conversion

[Moore, Robert]

"bool" can be problematic as it isn't totally portable. It is usually implemented as a macro.

That’s why ACPICA doesn't use it.


> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Monday, January 26, 2015 5:33 AM
> To: Quentin Lambert
> Cc: Zhang, Rui; Moore, Robert; Zheng, Lv; Wysocki, Rafael J; Len Brown;
> Shaohua Li; linux-acpi@vger.kernel.org; devel@acpica.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 0/4] int to bool conversion
> 
> On Monday, January 26, 2015 09:30:55 AM Quentin Lambert wrote:
> > Sorry for the delay in answering ....
> >
> > On 22/01/2015 17:18, Rafael J. Wysocki wrote:
> > > On Thursday, January 22, 2015 09:49:41 AM Quentin Lambert wrote:
> > >> These patches convert local variables from int to bool when relevant.
> > > And what exactly is the need for that?  Does that fix any functional
> problems?
> > >
> > >
> > It doesn't fix any functional problem. The point of this patch is to
> > increase the code readability by lifting some of the ambiguities that
> > appear when using an integer variable as boolean.
> >
> > My understanding is that by explicitly using a boolean declaration
> > when it is relevant it clearly informs the reader that the variable is
> > going to represent a binary state. Moreover, using the keywords true
> > and false help indicate that the variable will not be involved in any
> > computation other than boolean arithmetic.
> 
> Well, in the new code, yes.  The existing code is a different matter
> though and it doesn't actually hurt if you leave the ints where they are,
> so there's no reason to make those changes.
> 
> If you change old code and the change is not trivial (eg. fixes of white
> space or comments, or kernel messages etc.) and someone enounters a bug
> that may be related to it, they will have to go through your changes to
> see if that's not the source of the bug.  That's not really productive.
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.

RE: [PATCH 0/4] int to bool conversion

[Moore, Robert]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2202 bytes --]

"bool" can be problematic as it isn't totally portable. It is usually implemented as a macro.

That’s why ACPICA doesn't use it.


> -----Original Message-----
> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Sent: Monday, January 26, 2015 5:33 AM
> To: Quentin Lambert
> Cc: Zhang, Rui; Moore, Robert; Zheng, Lv; Wysocki, Rafael J; Len Brown;
> Shaohua Li; linux-acpi@vger.kernel.org; devel@acpica.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 0/4] int to bool conversion
> 
> On Monday, January 26, 2015 09:30:55 AM Quentin Lambert wrote:
> > Sorry for the delay in answering ....
> >
> > On 22/01/2015 17:18, Rafael J. Wysocki wrote:
> > > On Thursday, January 22, 2015 09:49:41 AM Quentin Lambert wrote:
> > >> These patches convert local variables from int to bool when relevant.
> > > And what exactly is the need for that?  Does that fix any functional
> problems?
> > >
> > >
> > It doesn't fix any functional problem. The point of this patch is to
> > increase the code readability by lifting some of the ambiguities that
> > appear when using an integer variable as boolean.
> >
> > My understanding is that by explicitly using a boolean declaration
> > when it is relevant it clearly informs the reader that the variable is
> > going to represent a binary state. Moreover, using the keywords true
> > and false help indicate that the variable will not be involved in any
> > computation other than boolean arithmetic.
> 
> Well, in the new code, yes.  The existing code is a different matter
> though and it doesn't actually hurt if you leave the ints where they are,
> so there's no reason to make those changes.
> 
> If you change old code and the change is not trivial (eg. fixes of white
> space or comments, or kernel messages etc.) and someone enounters a bug
> that may be related to it, they will have to go through your changes to
> see if that's not the source of the bug.  That's not really productive.
> 
> 
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/4] int to bool conversion

[Rafael J. Wysocki]

On Monday, January 26, 2015 09:30:55 AM Quentin Lambert wrote:
> Sorry for the delay in answering ....
> 
> On 22/01/2015 17:18, Rafael J. Wysocki wrote:
> > On Thursday, January 22, 2015 09:49:41 AM Quentin Lambert wrote:
> >> These patches convert local variables from int to bool when relevant.
> > And what exactly is the need for that?  Does that fix any functional problems?
> >
> >
> It doesn't fix any functional problem. The point of this patch
> is to increase the code readability by lifting some of the ambiguities
> that appear when using an integer variable as boolean.
> 
> My understanding is that by explicitly using a boolean declaration
> when it is relevant it clearly informs the reader that the variable
> is going to represent a binary state. Moreover, using the keywords
> true and false help indicate that the variable will not be involved
> in any computation other than boolean arithmetic.

Well, in the new code, yes.  The existing code is a different matter though
and it doesn't actually hurt if you leave the ints where they are, so there's
no reason to make those changes.

If you change old code and the change is not trivial (eg. fixes of white space
or comments, or kernel messages etc.) and someone enounters a bug that may be
related to it, they will have to go through your changes to see if that's not
the source of the bug.  That's not really productive.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 0/4] int to bool conversion

[Quentin Lambert]

Sorry for the delay in answering ....

On 22/01/2015 17:18, Rafael J. Wysocki wrote:
> On Thursday, January 22, 2015 09:49:41 AM Quentin Lambert wrote:
>> These patches convert local variables from int to bool when relevant.
> And what exactly is the need for that?  Does that fix any functional problems?
>
>
It doesn't fix any functional problem. The point of this patch
is to increase the code readability by lifting some of the ambiguities
that appear when using an integer variable as boolean.

My understanding is that by explicitly using a boolean declaration
when it is relevant it clearly informs the reader that the variable
is going to represent a binary state. Moreover, using the keywords
true and false help indicate that the variable will not be involved
in any computation other than boolean arithmetic.

That being said, I am new to kernel contribution and I feel that
a more compelling case may be made here:
http://lkml.iu.edu/hypermail/linux/kernel/0607.2/0791.html

BR,
Quentin

Re: [PATCH 0/4] int to bool conversion

[Rafael J. Wysocki]

On Thursday, January 22, 2015 09:49:41 AM Quentin Lambert wrote:
> These patches convert local variables from int to bool when relevant.

And what exactly is the need for that?  Does that fix any functional problems?


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 0/4] int to bool conversion

[Quentin Lambert]

These patches convert local variables from int to bool when relevant.

The first patch proposes straight forward cases where the conversion
do not suggest changes beyond the function.

The second patch deals with cases where the conversion is propagated
to function parameters.

The third patch is a single case where the variable is given as a
parameter to a kernel macro.

Finally, the fourth patch is a case in which the conversion is
propagated to the field of a structure.


Quentin Lambert (4):
  ACPI: Convert non-returned local variable to boolean when relevant
  ACPI: Convert int variable to bool and propagate to function
    parameters
  ACPI: Convert int to bool for variable later used int kernel macro
  ACPI: Convert int to bool and propagete to struct field

 drivers/acpi/acpi_pad.c         |  6 +++---
 drivers/acpi/acpi_processor.c   |  5 +++--
 drivers/acpi/acpica/acutils.h   |  3 ++-
 drivers/acpi/acpica/utaddress.c |  3 ++-
 drivers/acpi/acpica/utstring.c  | 10 +++++-----
 drivers/acpi/acpica/utxface.c   |  2 +-
 drivers/acpi/dock.c             |  4 ++--
 drivers/acpi/osl.c              |  4 ++--
 drivers/acpi/pci_link.c         |  4 ++--
 drivers/acpi/processor_core.c   | 12 ++++++------
 drivers/acpi/processor_pdc.c    |  5 +++--
 drivers/acpi/scan.c             |  4 ++--
 drivers/acpi/thermal.c          | 10 +++++-----
 13 files changed, 38 insertions(+), 34 deletions(-)

-- 
1.9.1