Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[john]

> > Saving a line over readability is utterly bogus.

> I agree 100%.  If you have anything more complex than
>
>         if (error) return (error);
>
> I want it to look like
>
>         if ((expr) || (expr2) || (expr3)) {
>                 return (error);
>         }

Ergh, personally I hate reading code like that.

Having said that, I hate code that is indented.  Am I the only person who mentally
counts, "in, in, in, out", etc, when reading code?  Artificial indenting really
throws off my concentration, because the 'prompting' it provides does nothing to
help me, but the ragged left margin is irritating, and makes moving code around
awkward, because you have to correct the indenting to match the current, (actual),
nesting level of the code, (because indenting is a completely artificial concept).

I'm not trying to say my coding style is right, and yours is wrong, I'm just curious
 :-).

John.

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[Matt Mackall]

On Sat, May 31, 2003 at 05:06:21PM +0100, john@grabjohn.com wrote:

> Having said that, I hate code that is indented. Am I the only person
> who mentally counts, "in, in, in, out", etc, when reading code?

Yes.
-- 
Matt Mackall : http://www.selenic.com : of or relating to the moon

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[Randy.Dunlap]

> On Sat, May 31, 2003 at 05:06:21PM +0100, john@grabjohn.com wrote:
>
>> Having said that, I hate code that is indented. Am I the only person who
>> mentally counts, "in, in, in, out", etc, when reading code?
>
> Yes.

I agree.  That's a "last resort" type of thing.

~Randy

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[viro]

On Sat, May 31, 2003 at 11:14:08AM -0600, Steven Cole wrote:

> statement when needed.
> 
> return -ETOSENDERADDRESSUNKNOWN;	/* this is OK */
> return (value & ZORRO_MASK);		/* so is this */

Like hell it is.  Parenthesis are _not_ needed here - production is
<statement> -> return <expression> ;

The only messy '('-related case in C grammar is sizeof as unary operation
vs. sizeof ( <type> ) (lovely way to torture parsers and students on exam:
sizeof (int)*p).  Everything else is pretty straightforward...

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[Steven Cole]

On Sat, 2003-05-31 at 09:39, Larry McVoy wrote:
> On Sat, May 31, 2003 at 04:01:50PM +0100, Dave Jones wrote:
> > Saving a line over readability is utterly bogus.
> 
> I agree 100%.  If you have anything more complex than
> 
> 	if (error) return (error);
> 
> I want it to look like
> 	
> 	if ((expr) || (expr2) || (expr3)) {
> 		return (error);
> 	}
> 
This may just be pedantic minutiae, but aren't those parenthesis around
"error" unnecessary?

Here is a proposal for coding style: Only use parenthesis in the return
statement when needed.

return -ETOSENDERADDRESSUNKNOWN;	/* this is OK */
return (value & ZORRO_MASK);		/* so is this */
return (-ENOTENOUGHCOFFEE);		/* bogus parenthesis */ 

Steven

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[Larry McVoy]

On Sat, May 31, 2003 at 04:01:50PM +0100, Dave Jones wrote:
> Saving a line over readability is utterly bogus.

I agree 100%.  If you have anything more complex than

	if (error) return (error);

I want it to look like
	
	if ((expr) || (expr2) || (expr3)) {
		return (error);
	}

> Just look at some of the crap we have in devfs..

No kidding, look at the nested if, that's insane.

>     if (fs_info->devfsd_task == NULL) return (TRUE);
>     if (devfsd_queue_empty (fs_info) && fs_info->devfsd_sleeping) return TRUE;
>     if ( is_devfsd_or_child (fs_info) ) return (FALSE);
>     set_current_state (TASK_UNINTERRUPTIBLE);
>     add_wait_queue (&fs_info->revalidate_wait_queue, &wait);
>     if (!devfsd_queue_empty (fs_info) || !fs_info->devfsd_sleeping)
>         if (fs_info->devfsd_task) schedule ();
>     remove_wait_queue (&fs_info->revalidate_wait_queue, &wait);
>     __set_current_state (TASK_RUNNING);
>     return (TRUE);

I took a pass at this, I think this is better (note the use of 1/2 tabs
as "continuation" lines, that's a Sun thing and it works pretty well:

	if ((fs_info->devfsd_task == NULL) ||
	    (devfsd_queue_empty(fs_info) && fs_info->devfsd_sleeping)) {
		return (TRUE);
	}
	if (is_devfsd_or_child(fs_info)) return (FALSE);
	set_current_state (TASK_UNINTERRUPTIBLE);
	add_wait_queue (&fs_info->revalidate_wait_queue, &wait);
	if ((!devfsd_queue_empty (fs_info) || !fs_info->devfsd_sleeping) &&
	    fs_info->devfsd_task) {
	    	schedule();
	}
	remove_wait_queue(&fs_info->revalidate_wait_queue, &wait);
	__set_current_state(TASK_RUNNING);
	return (TRUE);

-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm

Re: coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[Dave Jones]

On Sat, May 31, 2003 at 07:43:23AM -0700, Larry McVoy wrote:

 > One other one is the 
 > 
 > 	if (!q) return;
 > 
 > Chris said two lines, we don't do it that way.  The coding style we use is
 > a) one line is fine for a single statement.
 > b) in all other cases there are curly braces

Saving a line over readability is utterly bogus.
Just look at some of the crap we have in devfs..

    if (fs_info->devfsd_task == NULL) return (TRUE);
    if (devfsd_queue_empty (fs_info) && fs_info->devfsd_sleeping) return TRUE;
    if ( is_devfsd_or_child (fs_info) ) return (FALSE);
    set_current_state (TASK_UNINTERRUPTIBLE);
    add_wait_queue (&fs_info->revalidate_wait_queue, &wait);
    if (!devfsd_queue_empty (fs_info) || !fs_info->devfsd_sleeping)
        if (fs_info->devfsd_task) schedule ();
    remove_wait_queue (&fs_info->revalidate_wait_queue, &wait);
    __set_current_state (TASK_RUNNING);
    return (TRUE);

*horror* to my eyes at least.

Parts of the DRI code use similar uglies.  Whitespace is a *good* thing.
If you want more lines of code per screen, get a larger xterm, change a
font, whatever, but don't decrease code readability for something so bogus.

		Dave

coding style (was Re: [PATCH][2.5] UTF-8 support in console)

[Larry McVoy]

> Please linewrap after 80 chars.

Amen to that.

> +	if (!q) {
> 
> Kill the blank line above.
> 
> +		if (!q) return;
> 
> Two lines again.

A couple of comments: in the BK source tree, we've diverged from the Linux
coding style a bit (maybe a lot, Linus has read the source, ask him).

One thing is 

	unless (p) {
		....
	}
instead of 
	if (!p) {
		....
	}

It's just a
#define unless(x) if (!(x)) 
but it makes some code read quite a bit easier.  I'm a stickler for not using
2 lines where one will do, i.e.,

	FILE	*f;

	...
	unless (f = fopen(file, "r")) {
		error handling;
		return (-1);
	}

You hiccup the first time you see it, then you can read it, then you
start using it.  Yeah, I know, I'm using the value of an assignment in
a conditional, trust me, it works fine.

One other one is the 

	if (!q) return;

Chris said two lines, we don't do it that way.  The coding style we use is
a) one line is fine for a single statement.
b) in all other cases there are curly braces

	unless (q) return;	/* OK */
	unless (q) {		/* also OK */
		return;
	}
	unless (q)
		return;		/* not OK, no "}" */


The point of this style is twofold: save a line when the thing you are
doing is a singe statement, and make it easier for your eyes (or my 
tired old eyes) to run over the code.  If you see indentation you know
it is a block and there will be a closing } without exception.

It keeps the line counts about 10% smaller or so in our source base.
If you are looking for bragging rights about how big your stuff is that
might be bad but I like it because I can read more code in a window.
-- 
---
Larry McVoy              lm at bitmover.com          http://www.bitmover.com/lm