2007-05-01 09:00:38

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On Tue, 1 May 2007, Satyam Sharma wrote:
> Actually, the latter style (one condition per line and the && or ||
> operators appearing _before_ the conditions in subsequent lines)
> is quite popular for multi-line compound conditions (well, I've seen this
> in kernel/workqueue.c, kernel/stop_machine.c, etc at least, and also in
> Linus' code, if I'm not mistaken). We also align subsequent lines to the
> column of the condition belonging to the same "logical level" on the
> previous line using _spaces_ (note that this is alignment, not indentation,
> using spaces). The rationale is to make the operator prominent and thus make
> the structure of a complex multi-line compound conditional expression more
> readable and obvious at first glance itself. For example, consider:
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a ||
> condition3b)) {
> ...
> }
>
> versus
>
> if (veryverylengthycondition1
> && smallcond2
> && (conditionnumber3a
> || condition3b)) {
> ...
> }
>
> ?
>
> Latter wins, doesn't it?

... because you forgot to align subsequent lines to the column of the
condition belonging to the same "logical level" on the previous line.

Consider this:

if (veryverylengthycondition1 &&
smallcond2 &&
(conditionnumber3a ||
condition3b)) {
...
}

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds


2007-05-01 13:11:23

by Scott Preece

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On 5/1/07, Geert Uytterhoeven <[email protected]> wrote:
> On Tue, 1 May 2007, Satyam Sharma wrote:
> > Actually, the latter style (one condition per line and the && or ||
> > operators appearing _before_ the conditions in subsequent lines)
> > is quite popular for multi-line compound conditions (well, I've seen this
> > in kernel/workqueue.c, kernel/stop_machine.c, etc at least, and also in
> > Linus' code, if I'm not mistaken). We also align subsequent lines to the
> > column of the condition belonging to the same "logical level" on the
> > previous line using _spaces_ (note that this is alignment, not indentation,
> > using spaces). The rationale is to make the operator prominent and thus make
> > the structure of a complex multi-line compound conditional expression more
> > readable and obvious at first glance itself. For example, consider:
> >
> > if (veryverylengthycondition1 &&
> > smallcond2 &&
> > (conditionnumber3a ||
> > condition3b)) {
> > ...
> > }
> >
> > versus
> >
> > if (veryverylengthycondition1
> > && smallcond2
> > && (conditionnumber3a
> > || condition3b)) {
> > ...
> > }
> >
> > ?
> >
> > Latter wins, doesn't it?
>
> ... because you forgot to align subsequent lines to the column of the
> condition belonging to the same "logical level" on the previous line.
>
> Consider this:
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a ||
> condition3b)) {
> ...
> }
>
> Gr{oetje,eeting}s,
>
> Geert


I still find the leading-operator style much more readable. The most
important thing in reading a long, complex conditional is
understanding the structure of the operators, not the operands.
Putting the operators at the front of the line emphasizes that
structure, especially since you can line the operators up to match the
logical indenting, whereas the trailing-operator style leaves the
operators scattered and hard to assemble into a logical structure.

However, there's a lot of difference of opinion on this (perhaps
rooted in differences in cognition and reading behavior). For me it's
not even close - expressions broken so the operators are at the head
of the line snap into focus and those with operators at the ends of
the lines look like undifferentiated goo. Since some of the style
guides I've seen do it the other way, I assume some people have the
opposite perception. I guess that's why indent offers options for
doing it either way...

scott

2007-05-01 14:16:25

by David Woodhouse

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On Tue, 2007-05-01 at 11:00 +0200, Geert Uytterhoeven wrote:
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a ||
> condition3b)) {
> ...
> }

It's horrid. I'd much rather see

if (veryverylengthycondition1 &&
smallcond2 &&
(conditionnumber3a || condition3b)) {
...
}

Even if that does take it over 80 columns, the 'failure' mode will be
that it gets wrapped round to the beginning of the screen or truncated
at 80 columns -- and for the _majority_ of the time, it doesn't matter.

Unless you're paying particular attention to the logic and debugging the
statement, a 'high-level' view of what's going on is perfectly
sufficient; you don't actually read every character anyway.

--
dwmw2

2007-05-01 15:06:29

by John Anthony Kazos Jr.

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

> > if (veryverylengthycondition1 &&
> > smallcond2 &&
> > (conditionnumber3a ||
> > condition3b)) {
> > ...
> > }
>
> It's horrid. I'd much rather see
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a || condition3b)) {
> ...
> }

if (veryverylengthycondition1
&& smallcond2
&& (conditionnumber3a
|| condition3b)) {
...
}

Clear, crisp, and 80-wide. I also like how the logical operator on the
following line is indented slightly into the condition of the previous
line. I think this is much more sensical and sensible than using spaces to
line them up with the parentheses. Makes clear for each operator the
condition to which it applies.

2007-05-01 15:07:21

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On Tue, 1 May 2007, David Woodhouse wrote:
> On Tue, 2007-05-01 at 11:00 +0200, Geert Uytterhoeven wrote:
> >
> > if (veryverylengthycondition1 &&
> > smallcond2 &&
> > (conditionnumber3a ||
> > condition3b)) {
> > ...
> > }
>
> It's horrid. I'd much rather see
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a || condition3b)) {
> ...
> }

For clarity, if it fits, I prefer that one, too.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2007-05-01 15:07:37

by David Woodhouse

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On Tue, 2007-05-01 at 08:05 -0700, Randy Dunlap wrote:
> I prefer this format also, but I'm not sure that we can get it
> into CodingStyle. CodingStyle is about (either) concensus or
> dictum, but I don't see us close to concensus...

CodingStyle is mostly about consensus. We don't have a consensus, which
is why this particular stuff isn't specified in CodingStyle. :)

--
dwmw2

2007-05-01 15:08:48

by Randy Dunlap

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On Tue, 01 May 2007 15:16:13 +0100 David Woodhouse wrote:

> On Tue, 2007-05-01 at 11:00 +0200, Geert Uytterhoeven wrote:
> >
> > if (veryverylengthycondition1 &&
> > smallcond2 &&
> > (conditionnumber3a ||
> > condition3b)) {
> > ...
> > }
>
> It's horrid. I'd much rather see
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a || condition3b)) {
> ...
> }
>
> Even if that does take it over 80 columns, the 'failure' mode will be
> that it gets wrapped round to the beginning of the screen or truncated
> at 80 columns -- and for the _majority_ of the time, it doesn't matter.
>
> Unless you're paying particular attention to the logic and debugging the
> statement, a 'high-level' view of what's going on is perfectly
> sufficient; you don't actually read every character anyway.

I prefer this format also, but I'm not sure that we can get it
into CodingStyle. CodingStyle is about (either) concensus or
dictum, but I don't see us close to concensus...

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

2007-05-01 15:11:38

by David Woodhouse

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On Tue, 2007-05-01 at 17:07 +0200, Geert Uytterhoeven wrote:
> For clarity, if it fits, I prefer that one, too.

I don't think that was under question, was it?
My point was that I prefer it even when it _doesn't_ fit.

--
dwmw2

2007-05-01 16:13:12

by David Howells

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

John Anthony Kazos Jr. <[email protected]> wrote:

> if (veryverylengthycondition1
> && smallcond2
> && (conditionnumber3a
> || condition3b)) {
> ...
> }
>
> Clear, crisp, and 80-wide. I also like how the logical operator on the
> following line is indented slightly into the condition of the previous
> line.

The brain works better when associated things are lined up, so:

if (veryverylengthycondition1 &&
smallcond2 &&
(conditionnumber3a ||
condition3b)) {

is better as it gives a visual clue to association.

David

2007-05-01 20:12:23

by Satyam Sharma

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On 5/1/07, John Anthony Kazos Jr. <[email protected]> wrote:
> > It's horrid. I'd much rather see
> >
> > if (veryverylengthycondition1 &&
> > smallcond2 &&
> > (conditionnumber3a || condition3b)) {
> > ...
> > }
>
> if (veryverylengthycondition1
> && smallcond2
> && (conditionnumber3a
> || condition3b)) {
> ...
> }
>
> Clear, crisp, and 80-wide. I also like how the logical operator on the
> following line is indented slightly into the condition of the previous
> line. I think this is much more sensical and sensible than using spaces to
> line them up with the parentheses. Makes clear for each operator the
> condition to which it applies.

Hmmm ... I did use the spaces to line up the operators with the
starting column of the _expression_ in the previous lines, not the
parentheses (that would be gross!). Of course, && and || are binary
operators so they apply both to the expression on the previous line as
well as the expression following it on the same line.

2007-05-01 20:44:49

by Satyam Sharma

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

On 5/1/07, David Woodhouse <[email protected]> wrote:
> On Tue, 2007-05-01 at 08:05 -0700, Randy Dunlap wrote:
> > I prefer this format also, but I'm not sure that we can get it
> > into CodingStyle. CodingStyle is about (either) concensus or
> > dictum, but I don't see us close to concensus...

Yes, some of these styles are too personal and subjective to even try
and standardize. And then often even the same person doesn't follow a
single convention across his own code. More likely you'd succeed
standardizing *religion* than this ...

> CodingStyle is mostly about consensus. We don't have a consensus, which
> is why this particular stuff isn't specified in CodingStyle. :)

Actually, I'm not sure if we really gain much by finding consensus for
this particular stuff. Most compound conditions only contain upto 3-4
operators/expressions, so most of the styles discussed here would be
almost equally readable. And any code that goes beyond 3-4
operators/expressions is probably ugly in many other ways and needs to
fix its logic.

2007-05-02 02:25:04

by Eric W. Biederman

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

David Howells <[email protected]> writes:

> John Anthony Kazos Jr. <[email protected]> wrote:
>
>> if (veryverylengthycondition1
>> && smallcond2
>> && (conditionnumber3a
>> || condition3b)) {
>> ...
>> }
>>
>> Clear, crisp, and 80-wide. I also like how the logical operator on the
>> following line is indented slightly into the condition of the previous
>> line.
>
> The brain works better when associated things are lined up, so:
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a ||
> condition3b)) {
>
> is better as it gives a visual clue to association.

Not lining up with the code following the if statement is also
a plus. Because it clearly delineates the conditions from the code.

Of course in many cases conditions this complex are candidates for

if (!condition1)
continue;
if (!condition2)
continue;
if (!condition3a && !condition3b)
continue;

Eric

2007-05-02 09:33:37

by David Howells

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

Eric W. Biederman <[email protected]> wrote:

> Not lining up with the code following the if statement is also
> a plus. Because it clearly delineates the conditions from the code.

But the condition doesn't line up with the code:

if (veryverylengthycondition1 &&
smallcond2 &&
(conditionnumber3a ||
condition3b)) {
this_is_some_code();
this_is_some_more_code();
}

Personally, for complicated conditions like this, I prefer:

if (veryverylengthycondition1 &&
smallcond2 &&
(conditionnumber3a ||
condition3b)
) {
this_is_some_code();
this_is_some_more_code();
}

But that seems to offend Andrew for some reason (or was it Christoph? or
both?).

David

2007-05-02 11:56:31

by Eric W. Biederman

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

David Howells <[email protected]> writes:

> Eric W. Biederman <[email protected]> wrote:
>
>> Not lining up with the code following the if statement is also
>> a plus. Because it clearly delineates the conditions from the code.
>
> But the condition doesn't line up with the code:

Exactly. The condition not lining up with the following code helps
code helps separate the two.

> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a ||
> condition3b)) {
> this_is_some_code();
> this_is_some_more_code();
> }
>
> Personally, for complicated conditions like this, I prefer:
>
> if (veryverylengthycondition1 &&
> smallcond2 &&
> (conditionnumber3a ||
> condition3b)
> ) {
> this_is_some_code();
> this_is_some_more_code();
> }
>
> But that seems to offend Andrew for some reason (or was it Christoph? or
> both?).

Yes.

Although I suspect simply not tucking the trailing brace is as
good or better. I believe not putting the beginning brace at
the beginning of the line is a violation of coding style.

if (veryverylengthycondition1 &&
smallcond2 &&
(conditionnumber3a ||
condition3b))
{
this_is_some_code();
this_is_some_more_code();
}


However there is the practical way to solve this if you have
a sufficiently large conditional, or the conditional appears
several times.

static inline int test_func()
{
if (!veryverylengthycondition1)
return 0;
if (!smallcond2)
return 0;
if (conditionnumber3A)
return 1;
if (condition3b)
return 1;
return 0;
}

if (test_func()) {
this_is_some_code();
this_is_some_more_code();
}

Eric

2007-05-02 12:05:49

by David Howells

[permalink] [raw]
Subject: Re: condingstyle, was Re: utrace comments

Eric W. Biederman <[email protected]> wrote:

> > But the condition doesn't line up with the code:
>
> Exactly. The condition not lining up with the following code helps
> code helps separate the two.

Sorry about that: I realised you were agreeing with me about 5s after I sent
the message.

> However there is the practical way to solve this if you have
> a sufficiently large conditional, or the conditional appears
> several times.

That doesn't necessarily work - for instance if the condition has side effects
on local variables (eg: postinc). OTOH, large complex conditions with side
effects have to be abused with care and preferably avoided. goto is your
friend:-)

David