Coding style updates 02

Michael Adam ma at sernet.de
Fri Oct 19 10:10:01 GMT 2007


On Fri, Oct 19, 2007 at 11:42:25AM +0200, Stefan (metze) Metzmacher wrote:
> >> -If the beginning statement has to be broken across lines due to length,
> >> -the beginning brace should be on a line of its own.
> > 
> > I don't agree upon removing this rule. 
> > 
> > To my taste this is good:
> > 
> >> 	for (very_long_var_name_x=1;
> >> 	     very_long_var_name_x<10;
> >> 	     very_long_var_name_x++)
> >> 	{
> >> 		print("%d\n", very_long_var_name_x);
> >>  	}
> 
> ok, reverted this changes.


Thanks. (but see below in the patch for one more comment)

By the way: not commenting in the rest, I meant: +1 :-)

Michael

> From 50358050a41d0c574599f7678998143ad727f2c9 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 19 Oct 2007 11:39:05 +0200
> Subject: [Coding Style] add more examples about how braces should be used
> 
> metze
> ---
>  README.Coding |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 50 insertions(+), 6 deletions(-)
> 
> diff --git a/README.Coding b/README.Coding
> index fd52dbe..5b2cd5c 100644
> --- a/README.Coding
> +++ b/README.Coding
> @@ -145,15 +145,38 @@ The exception to the ending rule is when the closing brace is followed by
>  another language keyword such as else or the closing while in a do..while 
>  loop.
>  
> +The braces should only be omitted in special cases together with
> +return, goto, exit() or free().
> +
> +Also in if-else constructs braces should never be omitted.
> +
>  Good examples:
>  
>  	if (x == 1) {
>  		printf("good\n");
>  	}
>  
> -	for (x=1;
> -	     x<10;
> -	     x++)
> +	if (x == 1) {
> +		printf("good\n");
> +	} else if (x == 2) {
> +		printf("also good\n");
> +	} else {
> +		printf("bad\n");
> +	}
> +
> +	if (((very_long_var_name_x == 25) ||
> +	     (very_long_var_name_x > 10)) &&
> +	    (very_long_var_name_y == 1)) {

did this get in intentionally? it should be :

> +	if (((very_long_var_name_x == 25) ||
> +	     (very_long_var_name_x > 10)) &&
> +	    (very_long_var_name_y == 1)) 
> +	{

> +		printf("good\n");
> +	}
> +
> +	for (x=1; x<10; x++) {
> +		print("%d\n", x);
> +	}
> +
> +	for (very_long_var_name_x=1;
> +	     very_long_var_name_x<10;
> +	     very_long_var_name_x++)
>  	{
>  		print("%d\n", x);
>  	}
> @@ -162,6 +185,16 @@ Good examples:
>  		printf("also good\n");
>  	} while (1);
>  
> +
> +These should only be use together with return, goto, exit() or free():
> +
> +	if (x == 1) return 0;
> +
> +	if (some_long_function_name_check(x))
> +		goto failed;
> +
> +	if (ptr) free(ptr);
> +
>  Bad examples:
>  
>  	while (1)
> @@ -169,6 +202,18 @@ Bad examples:
>  		print("I'm in a loop!\n"); }
>  	
>  
> +	if (z)
> +		z++;
> +	else {
> +		z *= 2;
> +		z *= 3;
> +	}
> +
> +	if (z) {
> +		z++;
> +		z *= 4;
> +	} else z--;
> +
>  Goto
>  ----
>  
> @@ -179,7 +224,7 @@ is a goto outside of a function or block of code a good idea.
>  
>  Good Examples:
>  
> -int function foo(int y)
> +int foo(int y)
>  {
>  	int *z = NULL;
>  	int ret = 0;
> @@ -195,8 +240,7 @@ int function foo(int y)
>  	print("Allocated %d elements.\n", y);
>  
>   done: 
> -	if (z)
> -		free(z);
> +	if (z) free(z);
>  
>  	return ret;
>  }
> -- 
> 1.5.3.2
> 
> 
> From 1e2fd223407e85c70d06044c67cdc4e78d4c3f8f Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 19 Oct 2007 09:55:39 +0200
> Subject: [Coding Style] one statement per line is easier to review
> 
> metze
> ---
>  README.Coding |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/README.Coding b/README.Coding
> index 5b2cd5c..922c00a 100644
> --- a/README.Coding
> +++ b/README.Coding
> @@ -251,16 +251,17 @@ Checking Pointer Values
>  
>  When invoking functions that return pointer values, either of the following 
>  are acceptable.  Use you best judgement and choose the more readable option.
> -Remember that many other people will review it.
> +Remember that many other people will review it and having one logical
> +statement per line (1st example) is often easier to review.
>  
> -	if ((x = malloc(sizeof(short)*10)) == NULL ) {
> +	x = malloc(sizeof(short)*10);
> +	if (!x) {
>  		fprintf(stderr, "Unable to alloc memory!\n");
>  	}
>  
>  or
>  
> -	x = malloc(sizeof(short)*10);
> -	if (!x) {
> +	if ((x = malloc(sizeof(short)*10)) == NULL) {
>  		fprintf(stderr, "Unable to alloc memory!\n");
>  	}
>  
> -- 
> 1.5.3.2
> 
> 
> From 36fa6e3f03d325d6d0b80ada258fbe7d153aa4ca Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 19 Oct 2007 10:00:37 +0200
> Subject: [Coding Style] comments about integers and byte arrays
> 
> metze
> ---
>  README.Coding |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/README.Coding b/README.Coding
> index 922c00a..4fc06c6 100644
> --- a/README.Coding
> +++ b/README.Coding
> @@ -278,4 +278,7 @@ new code should adhere to the following conventions:
>  
>    * Booleans are of type "bool" (not BOOL)
>    * Boolean values are "true" and "false" (not True or False)
> +  * For intergers use "int" or "unsigned"
>    * Exact width integers are of type [u]int[8|16|32|64]_t
> +  * For byte arrays use "uint8_t *"
> +  * For sizes use "size_t" or "ssize_t"
> -- 
> 1.5.3.2
> 
> 
> From 3497e38a726ef3640d4eaca785ad5a20b8623883 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 19 Oct 2007 10:01:34 +0200
> Subject: [Coding Style] avoid typedefs and use meaningful names for structs and unions
> 
> metze
> ---
>  README.Coding |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/README.Coding b/README.Coding
> index 4fc06c6..97b2d4e 100644
> --- a/README.Coding
> +++ b/README.Coding
> @@ -282,3 +282,8 @@ new code should adhere to the following conventions:
>    * Exact width integers are of type [u]int[8|16|32|64]_t
>    * For byte arrays use "uint8_t *"
>    * For sizes use "size_t" or "ssize_t"
> +
> +Constructed Data Types
> +----------------------
> +
> +Try to avoid typedefs and choose meaningful struct and union names.
> -- 
> 1.5.3.2
> 
> 
> From 739a242e11108e9671a83826c028e3d610a70f82 Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Fri, 19 Oct 2007 10:02:21 +0200
> Subject: [Coding Style] Try to follow an "error and out" approach and avoid unneeded neesting.
> 
> metze
> ---
>  README.Coding |   90 +++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 84 insertions(+), 6 deletions(-)
> 
> diff --git a/README.Coding b/README.Coding
> index 97b2d4e..ed8c178 100644
> --- a/README.Coding
> +++ b/README.Coding
> @@ -229,12 +229,15 @@ int foo(int y)
>  	int *z = NULL;
>  	int ret = 0;
>  
> -	if ( y < 10 ) {
> -		z = malloc(sizeof(int)*y);
> -		if (!z) {
> -			ret = 1;
> -			goto done;
> -		}
> +	if ( y >= 10 ) {
> +		ret = 1;
> +		goto done;
> +	}
> +
> +	z = malloc(sizeof(int)*y);
> +	if (!z) {
> +		ret = 1;
> +		goto done;
>  	}
>  
>  	print("Allocated %d elements.\n", y);
> @@ -245,6 +248,81 @@ int foo(int y)
>  	return ret;
>  }
>  
> +Reduce the neesting levels
> +--------------------------
> +
> +Try to follow an "error and out" approach and avoid
> +unneeded neesting. Maybe move things to static helper
> +functions. Try to make the code reviewable even for C beginners.
> +
> +Good Examples:
> +
> +uint8_t *foo_talloc_array(TALLOC_CTX *mem_ctx, int x, int y)
> +{
> +	uint8_t *buf;
> +	size_t *blen;
> +
> +	if (x > 10) {
> +		printf("invalid parameter x = %d\n", x);
> +		return NULL;
> +	}
> +
> +	if (y > 10) {
> +		printf("invalid parameter y = %d\n", y);
> +		return NULL;
> +	}
> +
> +	blen = x * y;
> +	buf = talloc_size(mem_ctx, blen);
> +	if (!buf) {
> +		printf("talloc_size(%u) failed\n", blen);
> +		return NULL;
> +	}
> +
> +	return buf;
> +}
> +
> +Bad Examples:
> +
> +uint8_t *foo_talloc_array(TALLOC_CTX *mem_ctx, int x, int y)
> +{
> +	uint8_t *buf = NULL;
> +	size_t *blen;
> +
> +	if (x <= 10) {
> +		if (y <= 10) {
> +			blen = x * y;
> +			buf = talloc_size(mem_ctx, blen);
> +			if (!buf) {
> +				printf("talloc_size(%u) failed\n", blen);
> +			}
> +		} else {
> +			printf("invalid parameter y = %d\n", y);
> +		}
> +	} else {
> +		printf("invalid parameter x = %d\n", x);
> +	}
> +
> +	return buf;
> +}
> +
> +uint8_t *foo_talloc_array(TALLOC_CTX *mem_ctx, int x, int y)
> +{
> +	uint8_t *buf = NULL;
> +	size_t *blen;
> +
> +	if ((x > 10) || (y > 10)) {
> +		printf("invalid parameter x = %d y = %d\n", x, y);
> +	} else {
> +		blen = x * y;
> +		buf = talloc_size(mem_ctx, blen);
> +		if (!buf) {
> +			printf("talloc_size(%u) failed\n", blen);
> +		}
> +	}
> +
> +	return buf;
> +}
>  
>  Checking Pointer Values
>  -----------------------
> -- 
> 1.5.3.2
> 





-- 
Michael Adam <ma at sernet.de>
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.SerNet.DE, mailto: Info @ SerNet.DE


More information about the samba-technical mailing list