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