On Tue, Dec 21, 1999 at 10:22:12AM +0100, Ignatios Souvatzis wrote:
> On Tue, Dec 21, 1999 at 10:06:25AM +0900, Hayakawa Koichi wrote:
> > Hi, 
> > 
> > From: Lennart Augustsson <lennart@augustsson.net>
> > Subject: Cardbus
> > Date: Tue, 21 Dec 1999 01:50:54 +0100
> > Message-ID: <385ECEED.74713EBB@augustsson.net>
> > 
> >  >  * make the code KNF (this is easy, I can do that).
> > 
> > Please do that.
> > 
> > # I feel 8 columns for each indentation is too big.
> 
> Then the code structure is wrong... at least I was told this when I 
> felt 8 columns where too much, a couple of years ago.
> 
> I can elaborate, maybe, after my coffee.

So...

if you're coding like this:

function_head() {
	DECLARE_VARIABLES;

	if ((bla = alloc_this()) != NULL) {
		if ((initialize_that(foo)== 0)) {
			if (fd = open("that_file") >= 0) {
				operate_on_all_of_them;
				return SUCCESS;
			} else {
				perror("cannot open that_file");
			}
		} else {
			printf("initialize_that failed\n");
		}
	} else {
		printf("alloc_this failed\n");
	}
	return FAILURE;
}

you have used up 4 levels of indentation before any real work is done
in the function, and you have some of the error handling VERY far away
from the alloc/open/init that you check. The first problem forces you
to use smaller indents than 8 places per level. The first AND the second
problem makes the code very difficult to read and verify, and setting
indentation to 4 colums doesn't help much. If you don't believe me, read
the example code in the Amiga ROM Kernel Reference Manuals.

Now, if you are forced to use 8 columns per level, and forced to use
less than 80 columns, the only way out is to code like this:

function_head() {
	DECLARE_VARIABLES;

	if ((bla = alloc_this()) == NULL) {
		printf("alloc_this failed\n");
		return FAILURE_1;
	}

	if ((initialize_that(foo)== 0)) {
		printf("init_that failed\n");
		return FAILURE_2;
	}
	if (fd = open("that_file") == -1) {
		perror("cannot open that_file");
		return FAILURE_3;
	}

	operate_on_all_of_them;
	return SUCCESS;
}

so the main body of code is near the beginning of the line.

If you need to do cleanup on error, you do it like this:

function_head() {
	DECLARE_VARIABLES;
	int rc;

	rc = FAILURE;

	if ((bla = alloc_this()) == NULL) {
		printf("alloc_this failed\n");
		goto cleanup;
	}

	if ((initialize_that(foo)== 0)) {
		printf("init_that failed\n");
		goto cleanup;
	}
	if (fd = open("that_file") == -1) {
		perror("cannot open that_file");
		rc = FAILURE_3;
		goto cleanup;
	}

	operate_on_all_of_them;
	rc = SUCCESS;
cleaup:
	if (fd != -1)
		close(fd);

	if (initialized(foo))
		cleaup(foo);

	if (bla != NULL)
		free_this(bla);

	return rc;
}

Maybe our style guide should mention this explicitly.

Regards,
	Ignatios

-- 
 * Progress (n.): The process through which Usenet has evolved from
   smart people in front of dumb terminals to dumb people in front of
   smart terminals.  -- obs@burnout.demon.co.uk (obscurity)