trinity-devel@lists.pearsoncomputing.net

Message: previous - next
Month: January 2015

Re: [trinity-devel] Codebase formatting discussion

From: "Timothy Pearson" <kb9vqf@...>
Date: Fri, 9 Jan 2015 16:33:01 -0600
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA224

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 01/09/2015 11:40 AM, Timothy Pearson wrote:
>> <snip>
>
>> So, to be clear, this is what is desired?
>>
>> do_something(); switch(foo)  { case bar: a=1; case baz: a=2; case asd:
>> a=3; default: a=0; } do_something_else();
>>
>> My question is this:  Say I have a long case statement (dozens of cases
>> with subsequent processing blocks, enough
>> to run off both vertical edges of my screen) and it's "conveniently"
>> located in the middle of a large source file
>> with other case blocks and "normal code" above and below it. With the
>> style above I will have problems finding the
>> end of the case block, which is why I started indenting them like this:
>>
>> do_something(); switch(foo)  { case bar: a=1; case baz: a=2; case asd:
>> a=3; default: a=0; } do_something_else();
>>
>
> I think what is desired is something like this:
>
> do_something();
> switch(foo)  {
>     case bar:
>         a=1;
>         break;
>     case baz: {
>         a=2;
>         ...long case block...
>         c=4;
>         break;
>     }
>     case asd:
>         a=3;
>         break;
>     default:
>         a=0;
> }
> do_something_else();
>
> The { and } in case blocks are not really needed and my preference would
> be not to use them there. If needed, they
> should be indented as any other block. The style in GIT adds an extra
> indentation level to those {-} case blocks,
> which is what Slavek and I were "complaining".

Ah, OK, that is a valid complaint.  The case style shown above is fine and
we'll go with that.

My only remaining question then is should we also be forcing whitespace
between each case block for legibility, something like this?

do_something();
switch(foo)  {
    case bar:
        a=1;
        break;

    case baz: {
        a=2;
        ...long case block...
        c=4;
        break;
    }

    case asd:
        a=3;
        break;

    default:
        a=0;
}
do_something_else();

>> <snip> This is the current consensus as far as I can tell: 1.) Every
>> conditional uses braces 2.) Hard tabs 3.)
>> Conditional indentation like so:
>>
>> if (foo) { a=0; } else if (bar) { a=1; } else { a=2; }
>>
>> We still need to discuss how to handle large chains of conditionals,
>> e.g. these pathological cases:
>> https://git.trinitydesktop.org/cgit/tdelibs/tree/tdecore/tdehw/tdehardwaredevices.cpp?id=3d06098ece39d7a2e2d292b28fd0e165b3b4f43e#n2475
>>
>>
> Is the style shown there acceptable?
>
> My preference would be for breaking after the logical operator and either
> have no indentation or double indentation
> for the subsequent lines.
>
> if ((disktype & TDEDiskDeviceType::CDROM) ||
>    (disktype & TDEDiskDeviceType::CDR) ||
>    (disktype & TDEDiskDeviceType::CDRW))
>
> or
>
> if ((disktype & TDEDiskDeviceType::CDROM) ||
>        (disktype & TDEDiskDeviceType::CDR) ||
>        (disktype & TDEDiskDeviceType::CDRW))

I am curious as to why you find this more intuitive than having the
logical operator before the comparison.  Roughly translating to English
the above style reads something like:

if the box is black AND
<pause>
the box is not square AND
<pause>
the sphere is purple OR
the sphere is NOT PRESENT THEN

versus

if the box is black
<pause>
AND the box is not square
<pause>
AND the sphere is purple
<pause>
OR the sphere is NOT PRESENT
<pause>
THEN

You can see how in the latter style the relevant logical operator is
stated first, then the expression, with all required information to
understand the conditional contained on one line instead of spanning two
lines.  The latter is easier for me to grasp all of the conditions
required in a large expression; the former requires a bit more effort.

It isn't a big deal either way for me, and I'd like Slavek's input before
deciding, but I am curious as to the thought processes behind the
suggested style. :-)

>>
>> Also on the docket are how to handle simple variable assignments; there
>> are several ways I've seen: a=2; a = 2; a=
>> 2; a =2;
>>
>> I'm fairly undecided on this.  For consistency with larger statements "a
>> = 1;" should probably be used but it seems
>> such a waste of space compared with "a=1;".  Thoughts?
>
> Both "a=1" and "a = 1" are fine for me. For long time I used the first
> way, but in the last couple of years I switched
> ti the second one: usually more readable for simple expressions but can
> become confusing with long and complex ones
> such as:
> if ((a = 2) && (++b != (c + (x * y) / f(q,w,e))))
> Choice is up to you and Slavek.

I'm going to lean towards forcing spaces for consistency.  What is your
opinion Slavek?

>>
>> One other issue is the whitespace within conditional expressions.  As
>> you can see I'm fairly undecided on this as
>> well, having used multiple styles already:
>> https://git.trinitydesktop.org/cgit/tdelibs/tree/tdecore/tdehw/tdehardwaredevices.cpp?id=3d06098ece39d7a2e2d292b28fd0e165b3b4f43e#n2517
>>
>>
> https://git.trinitydesktop.org/cgit/tdelibs/tree/tdecore/tdehw/tdehardwaredevices.cpp?id=3d06098ece39d7a2e2d292b28fd0e165b3b4f43e#n2518
>>
>> Given a choice I slightly prefer the style shown on line 2518; my only
>> strong preference here is that I don't want
>> to see any whitespace between two matched parenthesis: Correct:
>> execute_something() Wrong:   execute_something( )
>>
>> Thoughts are welcome here as well.
>>
> Same as you, no spaces at the beginning and end parenthesis, i.e. line
> 2518 is better for me.

OK, we'll go with that then.

> Also, I suggest to have a space between function parameters as in
> foo(par1, par2, par3, par4)
> instead of
> foo(par1,par2,par3,par4)

Good catch!  Agreed.

As the number of finalized rules is steadily growing I have started an
Etherpad here to codify them and keep track of where we are at:
http://trinity.etherpad.trinitydesktop.org/87

Tim
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iFYEARELAAYFAlSwVwkACgkQLaxZSoRZrGFhIgDgusKW3eKuiRjYyne2oQ4kpkvV
iDD6LI4y4BQvrwDZAUvaXq2sksAPISFxzy+yvg+FE5GIzaBC6CiW+A==
=bXqC
-----END PGP SIGNATURE-----