Patchwork [v2,3/3] bitops: use bool

login
register
mail settings
Submitter Blue Swirl
Date July 8, 2012, 7:22 p.m.
Message ID <fb2b3c37220e9852f8083c15b6bbd99c5e59bbcc.1341775234.git.blauwirbel@gmail.com>
Download mbox | patch
Permalink /patch/169662/
State New
Headers show

Comments

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(-)
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

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)));
 }