Date: Mon, 29 Mar 2004 16:27:40 -0800
From: Paul Jackson <pj@sgi.com>
To: colpatch@us.ibm.com
Cc: linux-kernel@vger.kernel.org, mbligh@aracnet.com, akpm@osdl.org,
       wli@holomorphy.com, haveblue@us.ibm.com
Subject: Re: [PATCH] mask ADT: new mask.h file [2/22]
Message-Id: <20040329162740.0ca8f6d5.pj@sgi.com>
In-Reply-To: <1080606618.6742.89.camel@arrakis>
References: <20040329041253.5cd281a5.pj@sgi.com>
	<1080606618.6742.89.camel@arrakis>
Organization: SGI
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4386
Lines: 121

Matthew wrote (of my recommendation to not use the mask type directly):
> Is this necessary, or just convenient?

Technically as you suspect, just convenient, except in the case of the
mask_of_bit() macro, as you observe.

I was adhering to the K.I.S.S. school here - just tell the user one
recommended way of using things, suppressing my engineering urge to
explain alternatives that had no real advantages.

> This paragraph seems a bit unclear.

That's an understatment.  I left out a 'not' and didn't
explain half the cases.  See my updates, below.

> I think that it wouldn't be terribly ugly to split out the 1 unsigned
> long special cases (bitmap_and, bitmap_or, etc) with #ifdefs.

Do you have in mind an ifdef per function, or putting
several functions inside an ifdef?  If you think it
looks better - show us the code ;).

> test_and_set_bit((bit), (mask)._m) ?

yup.

Here's my cumulative patch of changes that I have gained so far
from your excellent feedback, and a couple I've noticed:

diff -Nru a/include/linux/mask.h b/include/linux/mask.h
--- a/include/linux/mask.h	Mon Mar 29 17:21:41 2004
+++ b/include/linux/mask.h	Mon Mar 29 17:21:41 2004
@@ -62,7 +62,7 @@
  *     Don't use mask.h directly - use cpumask.h and nodemask.h.
  *
  * The available mask operations and their rough meaning in the
- * case that "nbits == 8 * sizeof(single unsigned long)" are:
+ * case that "nbits == BITS_PER_LONG" are:
  *
  * __mask(nbits)			Use to define new *mask types
  * mask_setbit(bit, mask)		mask |= bit
@@ -70,7 +70,7 @@
  * mask_setall(mask, nbits)		mask = ~0UL
  * mask_clearall(mask)			mask = 0UL
  * mask_isset(bit, mask)		Is bit set in mask?
- * mask_test_and_set(bit, mask)		mask | bit ? 0 : mask |= bit
+ * mask_test_and_set(bit, mask)		mask & bit ? 1 : (mask |= bit, 0)
  * mask_and(dst, src1, src2)		dst = src1 & src2
  * mask_or(dst, src1, src2)		dst = src1 | src2
  * mask_xor(dst, src1, src2)		dst = src1 ^ src2
@@ -155,8 +155,10 @@
  *
  * The underlying bitmap.c operations such as bitmap_and() and
  *   bitmap_or() don't follow this model.  They don't assume
- *   the precondition that unused bits are zero, and they do
+ *   the precondition that unused bits are zero, and they do not
  *   mask off any unused portion of input masks in most cases.
+ *   The bitmap operations that produce Boolean or scalar results,
+ *   such as for empty, full and weight, _do_ filter out unused bits.
  *   However the underlying bitop.h operations, such as set_bit()
  *   and clear_bit(), do no sanitizing of their inputs, depending
  *   heavily on preconditions.
@@ -234,7 +236,7 @@
 	test_bit((bit), (mask)._m)
 
 #define mask_test_and_set(bit, mask)					\
-	test_and_set_bit(bit, (mask)._m)
+	test_and_set_bit((bit), (mask)._m)
 
 #define mask_and(dst, src1, src2)					\
 do {									\
diff -Nru a/lib/bitmap.c b/lib/bitmap.c
--- a/lib/bitmap.c	Mon Mar 29 17:21:41 2004
+++ b/lib/bitmap.c	Mon Mar 29 17:21:41 2004
@@ -47,7 +47,7 @@
 int bitmap_equal(const unsigned long *bitmap1,
 		const unsigned long *bitmap2, int bits)
 {
-	int k, lim = bits/BITS_PER_LONG;;
+	int k, lim = bits/BITS_PER_LONG;
 	for (k = 0; k < lim; ++k)
 		if (bitmap1[k] != bitmap2[k])
 			return 0;
@@ -63,7 +63,7 @@
 
 void bitmap_complement(unsigned long *dst, const unsigned long *src, int bits)
 {
-	int k, lim = bits/BITS_PER_LONG;;
+	int k, lim = bits/BITS_PER_LONG;
 	for (k = 0; k < lim; ++k)
 		dst[k] = ~src[k];
 
@@ -149,7 +149,7 @@
 int bitmap_intersects(const unsigned long *bitmap1,
 				const unsigned long *bitmap2, int bits)
 {
-	int k, lim = bits/BITS_PER_LONG;;
+	int k, lim = bits/BITS_PER_LONG;
 	for (k = 0; k < lim; ++k)
 		if (bitmap1[k] & bitmap2[k])
 			return 1;
@@ -165,7 +165,7 @@
 int bitmap_subset(const unsigned long *bitmap1,
 				const unsigned long *bitmap2, int bits)
 {
-	int k, lim = bits/BITS_PER_LONG;;
+	int k, lim = bits/BITS_PER_LONG;
 	for (k = 0; k < lim; ++k)
 		if (bitmap1[k] & ~bitmap2[k])
 			return 0;


-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/