[PATCH] include: introduce byte counting macros. (v2)

[Peter Hutterer]

This patch adds the following three macros:
 num_bytes_for_bits(bits) - the number of bytes needed to hold 'bits'
 num_dwords_for_bytes(bytes) - the number of 4-byte units to hold 'bytes'
 pad_to_dwords(bytes) - the closest multiple of 4 equal to or larger than 'bytes'.

All three operations are common in protocol processing and currently the
server has ((foo + 7)/8 + 3)/4 operations all over the place. A common set
of macros reduce the error rate of these (albeit simple) calculations and
improve readability of the code.

The macros do not check for overflow.

Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
---

Thanks for your comments!

On Tue, Jun 16, 2009 at 12:40:56PM +0200, Matthias Hopf wrote:
> - Macros should consist of capital letters - for a reason.
>   They tend to behave weird if the arguments are lvalues with side
>   effects (like i++). That said, these macros are save.

there's no particular reason for lowercase other than that I find it a lot
easier to read code that doesn't scream at me. I'm not too fussed about
uppercasing it though. FWIW, there's quite a few macros already that are
lowercase (swaps, min, max) or CamelCase (LengthRestB, WriteReplyToClient)
I don't think there's a lot with underscores, so maybe they should be
changed to CamelCase (which I'm not a particularly big fan of).

> - The names are not describing. count_bytes() counts bytes, but from
>   what? And it actually doesn't count.
>   Suggesting something like bytes_needed_for_bits(),
>   dwords_needed_for_bytes(), pad_to_dwords()

thanks, that was my biggest grief with the patch. How is:
    num_bytes_for_bits
    num_dwords_for_bytes
    pad_to_dwords
I think these are more descriptive. 

> - pad_to_dwords(bytes) would probably be better (((bytes) + 3) & ~3)
>   but that could result in the same code eventually if the compiler is
>   smart enough and the code equivalent enough. Which I doubt (negative
>   values?)

amended, see below.
divisions changed to >> 2 and >> 3.

> - They should all explicitly cast to unsigned. But that again makes it
>   difficult to use the same macros for different types (32/64). Dunno
>   what to do about that :-(

at some point I had that patch casting to (long long) before converting
since there's no >32 bit protocol value that'd require the use of these
macros (and most of these macros are useful for the protocol processing
code). This would also allow for values up to (including) INT_MAX, but I'm
not sure about whether that's a better approach.

Amended patch below.

Cheers,
  Peter

 include/misc.h |   20 ++++++++++++++++++++
 test/input.c   |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 0 deletions(-)

diff --git a/include/misc.h b/include/misc.h
index 61dd947..0edf8b3 100644
--- a/include/misc.h
+++ b/include/misc.h
@@ -180,6 +180,26 @@ typedef struct _xReq *xReqPtr;
 
 #endif
 
+/**
+ * Calculate the number of bytes needed to hold bits.
+ * @param bits The minimum number of bits needed.
+ * @return The number of bytes needed to hold bits.
+ */
+#define num_bytes_for_bits(bits) (((bits) + 7) >> 3)
+/**
+ * Calculate the number of 4-byte units needed to hold the given number of
+ * bytes.
+ * @param bytes The minimum number of bytes needed.
+ * @return The number of 4-byte units neede to hold bytes.
+ */
+#define num_dwords_for_bytes(bytes) (((bytes) + 3) >> 2)
+/**
+ * Calculate the number of bytes (in multiples of 4) needed to hold bytes.
+ * @param bytes The minimum number of bytes needed.
+ * @return The closest multiple of 4 that is equal or higher than bytes.
+ */
+#define pad_to_dwords(bytes) (((bytes) + 3) & ~3)
+
 /* some macros to help swap requests, replies, and events */
 
 #define LengthRestB(stuff) \
diff --git a/test/input.c b/test/input.c
index b80e1f5..c7a30fc 100644
--- a/test/input.c
+++ b/test/input.c
@@ -677,6 +677,44 @@ static void dix_grab_matching(void)
     g_assert(rc == TRUE);
 }
 
+static void include_byte_padding_macros(void)
+{
+    int i;
+    g_test_message("Testing num_bytes_for_bits()");
+
+    /* the macros don't provide overflow protection */
+    for (i = 0; i < INT_MAX - 7; i++)
+    {
+        int expected_bytes;
+        expected_bytes = (i + 7)/8;
+
+        g_assert(num_bytes_for_bits(i) >= i/8);
+        g_assert((num_bytes_for_bits(i) * 8) - i <= 7);
+    }
+
+    g_test_message("Testing num_dwords_for_bytes()");
+    for (i = 0; i < INT_MAX - 3; i++)
+    {
+        int expected_4byte;
+        expected_4byte = (i + 3)/4;
+
+        g_assert(num_dwords_for_bytes(i) <= i);
+        g_assert((num_dwords_for_bytes(i) * 4) - i <= 3);
+    }
+
+    g_test_message("Testing pad_to_dwords");
+
+    for (i = 0; i < INT_MAX - 3; i++)
+    {
+        int expected_bytes;
+        expected_bytes = ((i + 3)/4) * 4;
+
+        g_assert(pad_to_dwords(i) >= i);
+        g_assert(pad_to_dwords(i) - i <= 3);
+    }
+
+}
+
 int main(int argc, char** argv)
 {
     g_test_init(&argc, &argv,NULL);
@@ -687,6 +725,7 @@ int main(int argc, char** argv)
     g_test_add_func("/dix/input/check-grab-values", dix_check_grab_values);
     g_test_add_func("/dix/input/xi2-struct-sizes", xi2_struct_sizes);
     g_test_add_func("/dix/input/grab_matching", dix_grab_matching);
+    g_test_add_func("/include/byte_padding_macros", include_byte_padding_macros);
 
     return g_test_run();
 }
-- 
1.6.3.rc1.2.g0164.dirty