[v2,3/3] bitops: use bool

Submitted by Blue Swirl on July 8, 2012, 7:22 p.m.

Details

Message ID fb2b3c37220e9852f8083c15b6bbd99c5e59bbcc.1341775234.git.blauwirbel@gmail.com
State New
Headers show

Commit Message

Blue Swirl July 8, 2012, 7:22 p.m.
From: Blue Swirl <blauwirbel@gmail.com>

Use 'bool' type for return value of bit test functions.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 bitops.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Comments

Markus Armbruster July 9, 2012, 7:43 a.m.
blauwirbel@gmail.com writes:

> From: Blue Swirl <blauwirbel@gmail.com>
>
> Use 'bool' type for return value of bit test functions.

Matter of taste.  'bool' makes sense if you think of these functions as
predicates (ugly ones, with side effects).  'int' makes sense if you
think of them as returning the bit value (a bit is not a truth value in
my book).

In my opinion, this is just unnecessary churn, and unnecessary deviation
from established kernel practice.
Peter Maydell July 9, 2012, 8:14 a.m.
On 8 July 2012 20:22,  <blauwirbel@gmail.com> wrote:
> From: Blue Swirl <blauwirbel@gmail.com>
>
> Use 'bool' type for return value of bit test functions.
>
> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>

I'm 'meh' about the necessity for this change, but it looks safe
and functionally correct to me -- all these functions are either
already returning the result of a comparison, or (in the test_bit
case) a result that's only 0 or 1, so changing the result type
to bool can't break any callers.

-- PMM
Blue Swirl July 10, 2012, 7:32 p.m.
On Mon, Jul 9, 2012 at 7:43 AM, Markus Armbruster <armbru@redhat.com> wrote:
> blauwirbel@gmail.com writes:
>
>> From: Blue Swirl <blauwirbel@gmail.com>
>>
>> Use 'bool' type for return value of bit test functions.
>
> Matter of taste.  'bool' makes sense if you think of these functions as
> predicates (ugly ones, with side effects).  'int' makes sense if you
> think of them as returning the bit value (a bit is not a truth value in
> my book).

The documentation actually talks about returning the old value.

But I think the kernel version (with the same documentation) however
returns a truth value:
static inline int test_and_clear_bit(int nr, volatile unsigned long *addr)
{
	int oldbit;

	asm volatile(LOCK_PREFIX "btr %2,%1\n\t"
		     "sbb %0,%0"
		     : "=r" (oldbit), ADDR : "Ir" (nr) : "memory");

	return oldbit;
}

SBB oldbit, oldbit with oldbit = 0 initially and carry set will produce -1, no?

>
> In my opinion, this is just unnecessary churn, and unnecessary deviation
> from established kernel practice.

We use for example NetBSD queue macros with heavy modification. If we
have a need to modify code derived from Linux kernel, we should do it
as well. In this case the kernel practice seems to be inconsistent and
worth fixing.

Patch hide | download patch | download mbox

diff --git a/bitops.h b/bitops.h
index bc99727..1cecf00 100644
--- a/bitops.h
+++ b/bitops.h
@@ -153,7 +153,7 @@  static inline void change_bit(unsigned int nr, unsigned long *addr)
  * @nr: Bit to set
  * @addr: Address to count from
  */
-static inline int test_and_set_bit(unsigned int nr, unsigned long *addr)
+static inline bool test_and_set_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -168,7 +168,7 @@  static inline int test_and_set_bit(unsigned int nr, unsigned long *addr)
  * @nr: Bit to clear
  * @addr: Address to count from
  */
-static inline int test_and_clear_bit(unsigned int nr, unsigned long *addr)
+static inline bool test_and_clear_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -183,7 +183,7 @@  static inline int test_and_clear_bit(unsigned int nr, unsigned long *addr)
  * @nr: Bit to change
  * @addr: Address to count from
  */
-static inline int test_and_change_bit(unsigned int nr, unsigned long *addr)
+static inline bool test_and_change_bit(unsigned int nr, unsigned long *addr)
 {
 	unsigned long mask = BIT_MASK(nr);
 	unsigned long *p = ((unsigned long *)addr) + BIT_WORD(nr);
@@ -198,7 +198,7 @@  static inline int test_and_change_bit(unsigned int nr, unsigned long *addr)
  * @nr: bit number to test
  * @addr: Address to start counting from
  */
-static inline int test_bit(unsigned int nr, const unsigned long *addr)
+static inline bool test_bit(unsigned int nr, const unsigned long *addr)
 {
 	return 1UL & (addr[BIT_WORD(nr)] >> (nr & (BITS_PER_LONG-1)));
 }