diff mbox

[batman-adv] bitarray.[ch] Linux coding style cleanup

Message ID 20090714130926.GM19071@ma.tech.ascom.ch
State Accepted, archived
Headers show

Commit Message

Andrew Lunn July 14, 2009, 1:09 p.m. UTC 
Fix all the warnings generated by the 2.6.29 checkpatch script about
violations of the Linux coding style in the bitarray.[ch]
files.

Signed-off-by: Andrew Lunn <andrew.lunn@ascom.ch>

Comments

Andrew Lunn July 14, 2009, 1:20 p.m. UTC | #1 
On Tue, Jul 14, 2009 at 03:09:26PM +0200, Andrew Lunn wrote:
> Fix all the warnings generated by the 2.6.29 checkpatch script about
> violations of the Linux coding style in the bitarray.[ch]
> files.

This patch is to probe the waters a little. Marek has said off the
list that it has been decided that the batman-adv code should conform
to the Linux kernel coding standard. Much of the code is currently
not! What i dislike most in the current code base is the long
lines. The coding style says that lines should be less than 80
characters long. 

This patch cleans up two files, making them pass the coding style
checks for 2.6.29, the latest kernel code i have hanging around on my
machine.

What are peoples opinion about this? Should i spend a little time
cleaning up all the files? Some code needs refactoring a little, to
reducing the nesting, creating more helper functions, but mostly its
white space changes. However such changes can cause pain when trying
to merge in other work people who diff's are against before the
cleanup. Does anybody have any major patches?

         Thanks
                  Andrew
Antoine van Gelder July 14, 2009, 1:32 p.m. UTC | #2 
On 14 Jul 2009, at 15:20 , Andrew Lunn wrote:
> What are peoples opinion about this? Should i spend a little time
> cleaning up all the files? Some code needs refactoring a little, to
> reducing the nesting, creating more helper functions, but mostly its
> white space changes. However such changes can cause pain when trying
> to merge in other work people who diff's are against before the
> cleanup. Does anybody have any major patches?


I have a minor patch outstanding against the vis server but it  
wouldn't be affected by a cleanup.

Very happy with the decision to do a clean-up.

  - a
Sven Eckelmann July 14, 2009, 1:38 p.m. UTC | #3 
> On Tue, Jul 14, 2009 at 03:09:26PM +0200, Andrew Lunn wrote:
> What are peoples opinion about this? Should i spend a little time
> cleaning up all the files? Some code needs refactoring a little, to
> reducing the nesting, creating more helper functions, but mostly its
> white space changes. However such changes can cause pain when trying
> to merge in other work people who diff's are against before the
> cleanup. Does anybody have any major patches?
I am not a batman-adv developer but I am definitely pro cleanup. There aren't 
many known people which write code against the current version who wants to 
merge with trunk. And I think it is better to remove the clutter now instead 
of waiting until more "chaos" was created.
You should check out the scripts inside Andrew Morton's kernel[1,2]. I checked 
them some time ago and they were far better then the ones inside the vanilla 
kernel. I think most of that changes are already merged, but I don't know in 
which version.

[1] http://userweb.kernel.org/~akpm/mmotm/
[2] git://git.zen-sources.org/zen/mmotm.git
Andrew Lunn July 14, 2009, 1:57 p.m. UTC | #4 
> I have a minor patch outstanding against the vis server

Me too. I fixed the race condition at startup that you cannot enable
vis until batman is properly up and running.

> Very happy with the decision to do a clean-up.

The decision has not yet been made. I'm trying to gather opinions
about it...

      Andrew
Marek Lindner July 14, 2009, 2:48 p.m. UTC | #5 
On Tuesday 14 July 2009 21:20:04 Andrew Lunn wrote:
> What are peoples opinion about this? Should i spend a little time
> cleaning up all the files? Some code needs refactoring a little, to
> reducing the nesting, creating more helper functions, but mostly its
> white space changes. However such changes can cause pain when trying
> to merge in other work people who diff's are against before the
> cleanup. Does anybody have any major patches?

I have a major patch myself which I did not upload yet. It aims to improve the 
routing over multiple interfaces and thus touches quite some files. I'd rather 
test this stuff a bit longer before I screw up everyones routing.  ;-)

So, in my opinion there is not need to hesitate. Cleanup is a good idea.

Regards,
Marek
diff mbox

Patch

Index: batman-adv-kernelland/bitarray.c
===================================================================
--- batman-adv-kernelland/bitarray.c	(revision 1343)
+++ batman-adv-kernelland/bitarray.c	(working copy)
@@ -17,16 +17,10 @@ 
  *
  */
 
-
-
-
-
 #include "main.h"
 #include "bitarray.h"
 #include "log.h"
 
-
-
 /* clear the bits */
 void bit_init(TYPE_OF_WORD *seq_bits)
 {
@@ -36,8 +30,10 @@ 
 		seq_bits[i] = 0;
 }
 
-/* returns true if corresponding bit in given seq_bits indicates so and curr_seqno is within range of last_seqno */
-uint8_t get_bit_status(TYPE_OF_WORD *seq_bits, uint16_t last_seqno, uint16_t curr_seqno)
+/* returns true if the corresponding bit in the given seq_bits indicates true
+ * and curr_seqno is within range of last_seqno */
+uint8_t get_bit_status(TYPE_OF_WORD *seq_bits, uint16_t last_seqno,
+		       uint16_t curr_seqno)
 {
 	int16_t diff, word_offset, word_num;
 
@@ -45,8 +41,10 @@ 
 	if (diff < 0 || diff >= TQ_LOCAL_WINDOW_SIZE) {
 		return 0;
 	} else {
-		word_offset = (last_seqno - curr_seqno) % WORD_BIT_SIZE;/* which position in the selected word */
-		word_num = (last_seqno - curr_seqno) / WORD_BIT_SIZE;	/* which word */
+		/* which word */
+		word_num = (last_seqno - curr_seqno) / WORD_BIT_SIZE;
+		/* which position in the selected word */
+		word_offset = (last_seqno - curr_seqno) % WORD_BIT_SIZE;
 
 		if (seq_bits[word_num] & 1 << word_offset)
 			return 1;
@@ -57,22 +55,19 @@ 
 
 /* print the packet array, for debugging purposes */
 static char bit_string[130];
-char* bit_print(TYPE_OF_WORD *seq_bits) {
-	int i,j,k=0,b=0;
+char *bit_print(TYPE_OF_WORD *seq_bits)
+{
+	int i, j, k = 0, b = 0;
 
-// 	printf("the last %d packets, we got %d:\n", TQ_LOCAL_WINDOW_SIZE, bit_packet_count(seq_bits));
-	for ( i=0; i<NUM_WORDS; i++ ) {
-		for ( j=0; j<WORD_BIT_SIZE; j++) {
-			bit_string[k++] = ((seq_bits[i]>>j)%2 ? '1':'0'); /* print the j position */
-			if( ++b == TQ_LOCAL_WINDOW_SIZE ) {
-				bit_string[k++]='|';
-			}
+	for (i = 0; i < NUM_WORDS; i++) {
+		for (j = 0; j < WORD_BIT_SIZE; j++) {
+			bit_string[k++] = ((seq_bits[i]>>j)%2 ? '1' : '0');
+			if (++b == TQ_LOCAL_WINDOW_SIZE)
+				bit_string[k++] = '|';
 		}
-		bit_string[k++]=' ';
+		bit_string[k++] = ' ';
 	}
-	bit_string[k++]='\0';
-//	debug_output( 4, "%s\n", bit_string);
-//	printf("\n\n");
+	bit_string[k++] = '\0';
 	return bit_string;
 }
 
@@ -82,18 +77,18 @@ 
 	int32_t word_offset, word_num;
 
 	/* if too old, just drop it */
-	if (n<0 || n >= TQ_LOCAL_WINDOW_SIZE)
+	if (n < 0 || n >= TQ_LOCAL_WINDOW_SIZE)
 		return;
 
-// 	printf("mark bit %d\n", n);
+	/* which word */
+	word_num = n / WORD_BIT_SIZE;
+	/* which position in the selected word */
+	word_offset = n % WORD_BIT_SIZE;
 
-	word_offset = n % WORD_BIT_SIZE;	/* which position in the selected word */
-	word_num = n / WORD_BIT_SIZE;	/* which word */
-
 	seq_bits[word_num] |= 1 << word_offset;	/* turn the position on */
 }
 
-/* shift the packet array p by n places. */
+/* shift the packet array by n places. */
 void bit_shift(TYPE_OF_WORD *seq_bits, int32_t n)
 {
 	int32_t word_offset, word_num;
@@ -102,29 +97,36 @@ 
 	if (n <= 0)
 		return;
 
-	word_offset= n % WORD_BIT_SIZE;	/* shift how much inside each word */
+	word_offset = n % WORD_BIT_SIZE;/* shift how much inside each word */
 	word_num = n / WORD_BIT_SIZE;	/* shift over how much (full) words */
 
 	for (i = NUM_WORDS - 1; i > word_num; i--) {
-		/* going from old to new, so we can't overwrite the data we copy from. *
- 		 * left is high, right is low: FEDC BA98 7654 3210
+		/* going from old to new, so we don't overwrite the data we copy
+		 * from.
+		 *
+		 * left is high, right is low: FEDC BA98 7654 3210
 		 *	                                  ^^ ^^
 		 *                             vvvv
 		 * ^^^^ = from, vvvvv =to, we'd have word_num==1 and
-		 * word_offset==WORD_BIT_SIZE/2 ????? in this example. (=24 bits)
+		 * word_offset==WORD_BIT_SIZE/2 ????? in this example.
+		 * (=24 bits)
 		 *
 		 * our desired output would be: 9876 5432 1000 0000
 		 * */
 
 		seq_bits[i] =
 			(seq_bits[i - word_num] << word_offset) +
-					/* take the lower port from the left half, shift it left to its final position */
-			(seq_bits[i - word_num - 1] >> (WORD_BIT_SIZE-word_offset));
-					/* and the upper part of the right half and shift it left to it's position */
-		/* for our example that would be: word[0] = 9800 + 0076 = 9876 */
+			/* take the lower port from the left half, shift it left
+			 * to its final position */
+			(seq_bits[i - word_num - 1] >>
+			 (WORD_BIT_SIZE-word_offset));
+		/* and the upper part of the right half and shift it left to
+		 * it's position */
+		/* for our example that would be: word[0] = 9800 + 0076 =
+		 * 9876 */
 	}
-	/* now for our last word, i==word_num, we only have the it's "left" half. that's the 1000 word in
-	 * our example.*/
+	/* now for our last word, i==word_num, we only have the it's "left"
+	 * half. that's the 1000 word in our example.*/
 
 	seq_bits[i] = (seq_bits[i - word_num] << word_offset);
 
@@ -136,27 +138,33 @@ 
 }
 
 
-/* receive and process one packet, returns 1 if received seq_num is considered new, 0 if old  */
-char bit_get_packet(TYPE_OF_WORD *seq_bits, int16_t seq_num_diff, int8_t set_mark)
+/* receive and process one packet, returns 1 if received seq_num is considered
+ * new, 0 if old  */
+char bit_get_packet(TYPE_OF_WORD *seq_bits, int16_t seq_num_diff,
+		    int8_t set_mark)
 {
 	int i;
 
-	/* we already got a sequence number higher than this one, so we just mark it. this should wrap around the integer just fine */
+	/* we already got a sequence number higher than this one, so we just
+	 * mark it. this should wrap around the integer just fine */
 	if ((seq_num_diff < 0) && (seq_num_diff >= -TQ_LOCAL_WINDOW_SIZE)) {
 		if (set_mark)
 			bit_mark(seq_bits, -seq_num_diff);
-
 		return 0;
 	}
 
 	/* it seems we missed a lot of packets or the other host restarted */
-	if ((seq_num_diff > TQ_LOCAL_WINDOW_SIZE ) || (seq_num_diff < -TQ_LOCAL_WINDOW_SIZE)) {
+	if ((seq_num_diff > TQ_LOCAL_WINDOW_SIZE) ||
+	    (seq_num_diff < -TQ_LOCAL_WINDOW_SIZE)) {
 
 		if (seq_num_diff > TQ_LOCAL_WINDOW_SIZE)
-			debug_log(LOG_TYPE_BATMAN, "It seems we missed a lot of packets (%i) !\n",  seq_num_diff-1);
+			debug_log(LOG_TYPE_BATMAN,
+				  "We missed a lot of packets (%i) !\n",
+				  seq_num_diff-1);
 
 		if (-seq_num_diff > TQ_LOCAL_WINDOW_SIZE)
-			debug_log(LOG_TYPE_BATMAN, "Other host probably restarted !\n");
+			debug_log(LOG_TYPE_BATMAN,
+				  "Other host probably restarted !\n");
 
 		for (i = 0; i < NUM_WORDS; i++)
 			seq_bits[i] = 0;
@@ -173,35 +181,22 @@ 
 	return 1;
 }
 
-/* count the hamming weight, how many good packets did we receive? just count the 1's ... */
+/* count the hamming weight, how many good packets did we receive? just count
+ * the 1's. The inner loop uses the Kernighan algorithm, see
+ * http://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetKernighan
+ */
 int bit_packet_count(TYPE_OF_WORD *seq_bits)
 {
 	int i, hamming = 0;
 	TYPE_OF_WORD word;
 
 	for (i = 0; i < NUM_WORDS; i++) {
-
 		word = seq_bits[i];
 
 		while (word) {
-			word &= word-1;   /* see http://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetKernighan */
+			word &= word-1;
 			hamming++;
 		}
-
 	}
-
 	return hamming;
 }
-
-uint8_t bit_count(int32_t to_count)
-{
-	uint8_t hamming = 0;
-
-	while (to_count) {
-		to_count &= to_count - 1;   /* see http://graphics.stanford.edu/~seander/bithacks.html#CountBitsSetKernighan */
-		hamming++;
-	}
-
-	return hamming;
-}
-
Index: batman-adv-kernelland/bitarray.h
===================================================================
--- batman-adv-kernelland/bitarray.h	(revision 1343)
+++ batman-adv-kernelland/bitarray.h	(working copy)
@@ -18,18 +18,32 @@ 
  */
 
 
-
-#define TYPE_OF_WORD unsigned long /* you should choose something big, if you don't want to waste cpu */
+/* you should choose something big, if you don't want to waste cpu */
+#define TYPE_OF_WORD unsigned long
 #define WORD_BIT_SIZE (sizeof(TYPE_OF_WORD) * 8)
 
+/* clear the bits, ready for use */
+void bit_init(TYPE_OF_WORD *seq_bits);
 
+/* returns true if the corresponding bit in the given seq_bits indicates true
+ * and curr_seqno is within range of last_seqno */
+uint8_t get_bit_status(TYPE_OF_WORD *seq_bits, uint16_t last_seqno,
+					   uint16_t curr_seqno);
 
-void bit_init(TYPE_OF_WORD *seq_bits);
-uint8_t get_bit_status(TYPE_OF_WORD *seq_bits, uint16_t last_seqno, uint16_t curr_seqno);
-char *bit_print(TYPE_OF_WORD *seq_bits);
+/* turn corresponding bit on, so we can remember that we got the packet */
 void bit_mark(TYPE_OF_WORD *seq_bits, int32_t n);
+
+/* shift the packet array by n places. */
 void bit_shift(TYPE_OF_WORD *seq_bits, int32_t n);
-char bit_get_packet(TYPE_OF_WORD *seq_bits, int16_t seq_num_diff, int8_t set_mark);
+
+
+/* receive and process one packet, returns 1 if received seq_num is considered
+ * new, 0 if old  */
+char bit_get_packet(TYPE_OF_WORD *seq_bits, int16_t seq_num_diff,
+					int8_t set_mark);
+
+/* count the hamming weight, how many good packets did we receive? */
 int  bit_packet_count(TYPE_OF_WORD *seq_bits);
-uint8_t bit_count(int32_t to_count);
 
+/* print the packet array, for debugging purposes */
+char *bit_print(TYPE_OF_WORD *seq_bits);