CodingStyle question: multiple statements on a single line

CodingStyle question: multiple statements on a single line

[Hans Verkuil]

Hi all,

On the v4l-dvb list there has been confusion on how to interpret the 
following in the CodingStyle document:

"Don't put multiple statements on a single line unless you have 
something to hide:

        if (condition) do_this;
          do_something_everytime;

Don't put multiple assignments on a single line either.  Kernel coding 
style is super simple.  Avoid tricky expressions."

Does this mean:

1) Yes, after an 'if' you can put a single statement like this:

	if (a) b;

2) No, never use the 'if (a) b;' construction. Put 'b;' on the next line 
instead.

It's the 'have something to hide' part that confuses people: is this 
literal or is this sarcastic? This should be reworded to whatever is 
really meant.

And in general, why is this:

if (a) {
	b;
}

not accepted by the CodingStyle? (At least as I understand it)

Many people find this more readable and certainly safer. Certainly in 
cases like this:

for (...)
	for (...)
		statement;

I would really prefer to have curly brackets as that is much clearer. I 
would suggest that having curly brackets around a single statement is 
made optional. Especially in view of the sentence 'Avoid tricky 
expressions' I would say that this:

if (a)
	b;

is definitely trickier than:

if (a) {
	b;
}

All it takes is a single indentation error to get into trouble here.

Regards,

	Hans

Re: CodingStyle question: multiple statements on a single line

[Roland Dreier]

 > 2) No, never use the 'if (a) b;' construction. Put 'b;' on the next line 
 > instead.

This is correct.  Always write simple if statements as

	if (a)
		b;

 > And in general, why is this:
 > 
 > if (a) {
 > 	b;
 > }
 > 
 > not accepted by the CodingStyle? (At least as I understand it)

The braces take up another line of whitespace, which means less code
fits on the screen.  And in simple cases, they don't add anything.
Finally, the vast majority of the kernel leaves the braces off, so they
look funny to people who read a lot of kernel code.

And uniformity counts for a lot: most coding style rules are completely
arbitrary, but having a uniform kernel style makes reading kernel code
much easier.

Keep in mind that common sense always trumps any mechanical rule.  So if
there is some case where writing

	if (a) {
		b;
	}

is clearly easier to read than leaving the braces off, then that would
be OK.

 - R.

Re: CodingStyle question: multiple statements on a single line

[Mike Isely]

On Tue, 2 Sep 2008, Roland Dreier wrote:

>  > 2) No, never use the 'if (a) b;' construction. Put 'b;' on the next line 
>  > instead.
> 
> This is correct.  Always write simple if statements as
> 
> 	if (a)
> 		b;

Going back to the text:

<quote>
Don't put multiple statements on a single line unless you have
something to hide:
</quote>

Then what does "unless you have something to hide" refer to exactly?


<quote>
        if (condition) do_this;
          do_something_everytime;
</quote>

Realize that "if (condition) do_this;" is in fact one statement.  The 
"if (condition)" part is not something that can stand on its own; it is 
a predicate which is not completed without the rest of the statement.  
I interpret the example to be showing what is correct, not what is 
disallowed.  Both lines are single statements, each on their own line.  
I see the above as trying to illustrate outlawing of this sort of 
silliness:

        if (condition) do_this; do_something_everytime;

which will compile and run but is obviously misleading.

If anything the "do_this" is an example where one has something to hide, 
since it is not in the normal flow of execution (predicated instead by 
"if (condition)").

If anything,

	if (condition) do_this;

is safer than

	if (conditon)
		do_this;

because while in both cases it's one statement, the second case is split 
in exactly the spot where somebody's later errant line of code (say, a 
debug printk) will completely change what is going on.  With the whole 
statement on one line, this situation is avoided.  Or perhaps

	if (condition) {
		do_this;
	}

is another way to avoid the same issue, but that seems frowned upon for 
other reasons (below).

I know the answer to all this is just "but nobody does it that way".  
But my reading of the CodingStyle says this is allowed and that is what 
I thought was being asked - what does CodingStyle say?


> 
>  > And in general, why is this:
>  > 
>  > if (a) {
>  > 	b;
>  > }
>  > 
>  > not accepted by the CodingStyle? (At least as I understand it)
> 
> The braces take up another line of whitespace, which means less code
> fits on the screen.  And in simple cases, they don't add anything.
> Finally, the vast majority of the kernel leaves the braces off, so they
> look funny to people who read a lot of kernel code.

So by that reasoning "if (a) b;" - provided it stays under 80 columns - 
should be even better.  It occupies less space so that more code can fit 
on the screen.


> 
> And uniformity counts for a lot: most coding style rules are completely
> arbitrary, but having a uniform kernel style makes reading kernel code
> much easier.

What about drivers?  The statement has been made by others that there is 
a strong desire for outside drivers to be brought into mainline rather 
than being out-of-tree.  So must every chunk of code brought in this way 
be sanitized to this level of detail?  In many cases that can be a large 
(and some might say arbitrary) hurdle.


> 
> Keep in mind that common sense always trumps any mechanical rule.  So if
> there is some case where writing
> 
> 	if (a) {
> 		b;
> 	}
> 
> is clearly easier to read than leaving the braces off, then that would
> be OK.

That's great.  How does one reconcile this statement with subsystem 
maintainers who treat checkpatch.pl - which is the epitome of 
"mechanical rule" and has no notion of common sense - as the gatekeeper 
for all incoming changesets?

  -Mike


--

Mike Isely
isely @ pobox (dot) com
PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8

Re: CodingStyle question: multiple statements on a single line

[Randy Dunlap]

On Tue, 2 Sep 2008 18:50:44 -0500 (CDT) Mike Isely wrote:

> On Tue, 2 Sep 2008, Roland Dreier wrote:
> 
> >  > 2) No, never use the 'if (a) b;' construction. Put 'b;' on the next line 
> >  > instead.
> > 
> > This is correct.  Always write simple if statements as
> > 
> > 	if (a)
> > 		b;
> 
> Going back to the text:
> 
> <quote>
> Don't put multiple statements on a single line unless you have
> something to hide:
> </quote>
> 
> Then what does "unless you have something to hide" refer to exactly?

IMO it means "don't do it".  I.e., you/we shouldn't have anything to hide.


> <quote>
>         if (condition) do_this;
>           do_something_everytime;
> </quote>
> 
> Realize that "if (condition) do_this;" is in fact one statement.  The 
> "if (condition)" part is not something that can stand on its own; it is 
> a predicate which is not completed without the rest of the statement.  
> I interpret the example to be showing what is correct, not what is 

I disagree FWIW.  It's a deliberate bad example AFAIK.

> disallowed.  Both lines are single statements, each on their own line.  
> I see the above as trying to illustrate outlawing of this sort of 
> silliness:
> 
>         if (condition) do_this; do_something_everytime;
> 
> which will compile and run but is obviously misleading.
> 
> If anything the "do_this" is an example where one has something to hide, 
> since it is not in the normal flow of execution (predicated instead by 
> "if (condition)").
> 
> If anything,
> 
> 	if (condition) do_this;
> 
> is safer than
> 
> 	if (conditon)
> 		do_this;
> 
> because while in both cases it's one statement, the second case is split 
> in exactly the spot where somebody's later errant line of code (say, a 
> debug printk) will completely change what is going on.  With the whole 
> statement on one line, this situation is avoided.  Or perhaps

I suppose anyone can screw anything up.  :(


> 
> 	if (condition) {
> 		do_this;
> 	}
> 
> is another way to avoid the same issue, but that seems frowned upon for 
> other reasons (below).
> 
> I know the answer to all this is just "but nobody does it that way".  
> But my reading of the CodingStyle says this is allowed and that is what 
> I thought was being asked - what does CodingStyle say?
> 
> 
> > 
> >  > And in general, why is this:
> >  > 
> >  > if (a) {
> >  > 	b;
> >  > }
> >  > 
> >  > not accepted by the CodingStyle? (At least as I understand it)
> > 
> > The braces take up another line of whitespace, which means less code
> > fits on the screen.  And in simple cases, they don't add anything.
> > Finally, the vast majority of the kernel leaves the braces off, so they
> > look funny to people who read a lot of kernel code.
> 
> So by that reasoning "if (a) b;" - provided it stays under 80 columns - 
> should be even better.  It occupies less space so that more code can fit 
> on the screen.

It's too close to hidden IMO.

> > 
> > And uniformity counts for a lot: most coding style rules are completely
> > arbitrary, but having a uniform kernel style makes reading kernel code
> > much easier.
> 
> What about drivers?  The statement has been made by others that there is 
> a strong desire for outside drivers to be brought into mainline rather 
> than being out-of-tree.  So must every chunk of code brought in this way 
> be sanitized to this level of detail?  In many cases that can be a large 
> (and some might say arbitrary) hurdle.

It's not difficult.  It's not a big hurdle.

> > 
> > Keep in mind that common sense always trumps any mechanical rule.  So if
> > there is some case where writing
> > 
> > 	if (a) {
> > 		b;
> > 	}
> > 
> > is clearly easier to read than leaving the braces off, then that would
> > be OK.
> 
> That's great.  How does one reconcile this statement with subsystem 
> maintainers who treat checkpatch.pl - which is the epitome of 
> "mechanical rule" and has no notion of common sense - as the gatekeeper 
> for all incoming changesets?

Use a clue-by-four?  The checkpatch.pl maintainers know that it is
just a tool.  There are numerous exceptional cases.
Use common sense.


---
~Randy
Linux Plumbers Conference, 17-19 September 2008, Portland, Oregon USA
http://linuxplumbersconf.org/

Re: CodingStyle question: multiple statements on a single line

[Frans Meulenbroeks]

2008/9/3 Hans Verkuil <hverkuil@xs4all.nl>:
[...]
>
> "Don't put multiple statements on a single line unless you have
> something to hide:
>
>        if (condition) do_this;
>          do_something_everytime;
>
> Don't put multiple assignments on a single line either.  Kernel coding
> style is super simple.  Avoid tricky expressions."
>
> Does this mean:
>
> 1) Yes, after an 'if' you can put a single statement like this:
>
>        if (a) b;
>
> 2) No, never use the 'if (a) b;' construction. Put 'b;' on the next line
> instead.
>

It means 2. The example is definitely tricky. At first sight the
indentation suggests that the 'do_something-everytime" belongs to the
if, so it might put people on the wrong foot.
Suggest the text to be revised though. Some of us are better in
English than others. No need to cause confusion...

just my two eurocents :-)

Frans

[...]

Re: CodingStyle question: multiple statements on a single line

[Krzysztof Halasa]

Roland Dreier <rdreier@cisco.com> writes:

> This is correct.  Always write simple if statements as
>
> 	if (a)
> 		b;

The CodingStyle should never say "never" (nor "always").

Consider:

	if (a)		b;
	else if (c)	d;
	else if (e)	f;
	etc.

If the list is long and expressions simple this is IMHO much more
readable. The same with

	switch (a) {
	case b: c; break;
	case d: e; break;
	and so on.

> Keep in mind that common sense always trumps any mechanical rule.

Precisely.
-- 
Krzysztof Halasa

Re: CodingStyle question: multiple statements on a single line

[Krzysztof Halasa]

Mike Isely <isely@isely.net> writes:

> That's great.  How does one reconcile this statement with subsystem 
> maintainers who treat checkpatch.pl - which is the epitome of 
> "mechanical rule" and has no notion of common sense - as the gatekeeper 
> for all incoming changesets?

They need a doctor ASAP. But I don't know of any such maintainer.
-- 
Krzysztof Halasa

Re: CodingStyle question: multiple statements on a single line

[Roland Dreier]

 > <quote>
 > Don't put multiple statements on a single line unless you have
 > something to hide:
 > </quote>
 > 
 > Then what does "unless you have something to hide" refer to exactly?

I think it's a tongue-in-cheek way of saying "unless you're trying to
make your code hard to read."  In other words, don't do it.

 > So by that reasoning "if (a) b;" - provided it stays under 80 columns - 
 > should be even better.  It occupies less space so that more code can fit 
 > on the screen.

But it is visually hard to distinguish between the condition (a) and the
statement (b) that follows it.  And the arbitrary kernel rule is that we
don't use that style.

 > > And uniformity counts for a lot: most coding style rules are completely
 > > arbitrary, but having a uniform kernel style makes reading kernel code
 > > much easier.

 > What about drivers?  The statement has been made by others that there is 
 > a strong desire for outside drivers to be brought into mainline rather 
 > than being out-of-tree.  So must every chunk of code brought in this way 
 > be sanitized to this level of detail?  In many cases that can be a large 
 > (and some might say arbitrary) hurdle.

Of course having a uniform coding style is important for drivers too.
There are many cases where someone other than the original author is
debugging a driver and needs to read and understand the code, or where
someone is updating all in-tree drivers to cope with an API change and
must read and correctly change lots of driver code.

In general we are pretty lenient about driver code, as long as it pretty
much looks like kernel code.  But really, how much work is it to make a
one-time sweep over a driver and change

	if (a) {
		b;
	}

to

	if (a)
		b;

especially when you have an automated tool (checkpatch.pl) that can help
you find the places that would change?

 > That's great.  How does one reconcile this statement with subsystem 
 > maintainers who treat checkpatch.pl - which is the epitome of 
 > "mechanical rule" and has no notion of common sense - as the gatekeeper 
 > for all incoming changesets?

I don't know of any such maintainers.  In general if someone submitted a
patch that only triggered a few checkpatch warnings and gave a rationale
about why the remaining warnings shouldn't be worried about, then that
would be fine with me.

 - R.

Re: CodingStyle question: multiple statements on a single line

[Lennart Sorensen]

On Wed, Sep 03, 2008 at 11:38:01AM -0700, Roland Dreier wrote:
> 	if (a) {
> 		b;
> 	}
> 
> to
> 
> 	if (a)
> 		b;
> 

Why oh why the kernel do this stupid style?

I hate that.  It makes debuging such a pain in the @#$@#$.  Anytime you
want to do anything in that area you have to add back the missing '{}'
around the statement, so that you can add debug statements to the
condition.

Stupid stupid stupid!!!

It is so annoying when you accidentally break the code by trying to
debug it by doing:

	if (a)
		printk("Trying to do b\n");
		b;

Usually kernel style makes sense, but this part is stupid, inconsistend
with how any condition with multiple statements is done, and error
prone, especially when trying to debug.

Who's stupid idea was this anyhow?

-- 
Len Sorensen