Angband Coding Style

[calris]

Looking at the coding style for Angband, I notice it's pretty much identical to the Linux coding style except for two minor differences:

• Angband uses 4 tabs, Linux uses 8
• Angband permits 'break' and 'continue' to be placed on the same line as conditional statements

Personally, I prefer 8 tabs over 4, but I'm happy to work with 4

Allowing 'break' and 'continue' to share the same line as a conditional statement, however, just feels wrong. From the example in the coding guidelines:

            /* Only print even numbers */
            if (i % 2) continue;

            /* Be clever */
            printf("Aha!");

I've actually been caught out by code like this - thinking that the printf would get called when (i % 2) was non-zero.

To me, this:

            /* Only print even numbers */
            if (i % 2)
                    continue;

            /* Be clever */
            printf("Aha!");

is much clearer. Also, when the if condition is long and complex, it's easy to miss that there is actually a statement dangling off the end. Add to this that this 'exception' has been abused and there is code other than 'break' and 'continue' that has crept in, I think we should drop the exception entirely.

The only other 'issue' I have is breaking function parameters over multiple lines. When a function declaration or call hits 80 columns, we need to split the parameters over multiple lines. The Linux coding style does not cover specifically how this is handled - Angband's coding style does.

Personally, I prefer this:

         foo(buf,
             sizeof(buf),
             FLAG_UNUSED,
             FLAG_TIMED,
             FLAG_DEAD);

Over this (which is the current Angband coding style):

         foo(buf, sizeof(buf), FLAG_UNUSED,
              FLAG_TIMED, FLAG_DEAD);

Partly because if we add another parameter, we end up with:

         foo(buf,
             sizeof(buf),
             new_parameter,
             FLAG_UNUSED,
             FLAG_TIMED,
             FLAG_DEAD);

Which, when you look at the patch, the addition of the new parameter is clear and precise

Compared to this:

         foo(buf, sizeof(buf), new_parameter,
              FLAG_UNUSED, FLAG_TIMED, FLAG_DEAD);

Which looks plain ugly in a patch.

Lastly, I'm wondering what you all think of the following:

        if (some_variable)
                do_something();
        else
                do_something_else();

So far, so good - but what about when we add a comment? Do we do this:

        if (some_variable)
                /* Ideally we want this to happen */
                do_something();
        else
                /* But we have a fallback strategy */
                do_something_else();

Or this?

        if (some_variable) {
                /* Ideally we want this to happen */
                do_something();
        } else {
                /* But we have a fallback strategy */
                do_something_else();
        }

To be honest, I find both options a bit ugly - single-statement if/then results should be so obvious as to never need a comment

[Derakon]

Re: the latter, IMO if your conditionals, for loops, etc. do not have curly braces, then your code is buggy. If not now, then later when someone modifies it. Always always always use curly braces, no matter how simple the use case.

[calris]

Re: the latter, IMO if your conditionals, for loops, etc. do not have curly braces, then your code is buggy. If not now, then later when someone modifies it. Always always always use curly braces, no matter how simple the use case.

I am more than happy to apply this as an enforced style

But I'm just the monkey turning the handle - I'll do whatever the consensus tell me to do

[calris]

I've modified checkpatch.pl to optionally allow or forbid single statement blocks to use braces, so it will be fairly trivial to go through and clean them all up.

[PowerWyrm]

Always always always use curly braces, no matter how simple the use case.

Reminds me of an old bug in the code that took me forever to catch. It was something like: if (condition) do_something() where do_something was actually a multiline macro defined elsewhere...

[calris]

Reminds me of an old bug in the code that took me forever to catch. It was something like: if (condition) do_something() where do_something was actually a multiline macro defined elsewhere...

So we are agreed - always use curly braces? Unless someone gives me a compelling reason, I'll implement that from the next patch

EDIT: Linux avoids this problem by using do { } while (0) fancies. There are other reasons for using that construct, but it has the added benifit...

[takkaria]

calris, I think all your suggestions make sense. I also see the argument for always having braces though I'm not bothered either way, since I prefer to see more on my screen when possible, especially in a language as unexpressive as C.

I noticed in your branch you're reindenting all the switch statements differently, so the 'case's are on the same vertical line as the 'switch'es. This isn't mentioned in the coding guidelines for some reason but Angband house style has always been to indent the cases an extra tab.

[calris]

calris, I think all your suggestions make sense. I also see the argument for always having braces though I'm not bothered either way, since I prefer to see more on my screen when possible, especially in a language as unexpressive as C.

I know Linux favours no braces, but personally I think the risk outweighs the reward. And it's only one lost line as the opening brace is on the same line as the conditional.

I noticed in your branch you're reindenting all the switch statements differently, so the 'case's are on the same vertical line as the 'switch'es. This isn't mentioned in the coding guidelines for some reason but Angband house style has always been to indent the cases an extra tab.

Eeep - it certainly helps to not waste an intire level of indent. But if I drop from 8 space tabs to 4, we get it back anyway. I'm happy to indent the 'case's - I will need to look at how to pick it up in checkpatch.pl

[molybdenum]

I too am in favor of always using braces. In addition to the already-mentioned reasons, it's less of a pain to add new lines or just jam printf everywhere for debugging.

I'll throw another one out as well. I'm not a fan of this style for branches:

if (x) {
    ...
} else if (y) {
    ...
} else {
    ...
}

mainly because it collapses down to something like this:

if (x) {} else if (y) {} else {}

which makes it harder to see how many branches there are (since they're all on one line which is probably too long). I prefer just moving the else branches to the next line, so that things collapse like so:

if (x) {}
else if (y) {}
else {}

You can see the conditions better, it's easier to expand just one branch and, to me, it is more of a visual chunk than a single line.

[calris]

mainly because it collapses down to something like this:

if (x) {} else if (y) {} else {}

which makes it harder to see how many branches there are (since they're all on one line which is probably too long).

I don't understand - how does it 'collapse down'?

I prefer just moving the else branches to the next line, so that things collapse like so:

if (x) {}
else if (y) {}
else {}

Putting code on the same line as the condition is completely wrong. You end up with

x = some_sane_value;
if (some really long expression thats uses x and is almost 80 colums) {i++}

x = some_array[i];

some_function (x);

There are some truely horrible abuses of style regarding putting 'simple' statements on the same line as the condition.

You can see the conditions better, it's easier to expand just one branch and, to me, it is more of a visual chuk than a single line.

You can see the code associated with a condition better if it's directly underneath (indented ny one tab) - that way, all the code for every condition has identical left allignment

[Nick]

I am in an interesting position with respect to this discussion. On one hand, I'm a latecomer to C programming, and don't have very strong feelings about style - I have in fact learnt a lot of my habits from first Oangband and now V.

On the other hand, in recent times I've probably been the person writing the most V code, so I'm the most directly affected by a change to coding style.

Given that, I reckon the rest of you (and anyone else who wants to get involved) should slug it out here, and report back to me when you're done. How does that sound?

[molybdenum]

I don't understand - how does it 'collapse down'?

Sorry, I was referring to code folding; I use it all the time. I was trying to give an example of why I prefer to have a line break between the } and the else. Otherwise, I totally agree with what you are saying.

[Derakon]

IMO control flow keywords should generally be the first non-whitespace on their line, with the possible exception of a do-while loop.

[calris]

IMO control flow keywords should generally be the first non-whitespace on their line, with the possible exception of a do-while loop.

Here's a simple example of all the variants for the sake of comparisons

Style A - I think we can all agree this one is bad

    if (test_1) i++;
    else if (test_2) j++;
    else k++;

Style B (Linux Style) - Not as bad, but prone to introduce new bugs and adding a single statement to any line creates a patch which involves adding braces all over the place

    if (test_1)
        i++;
    else if (test_2)
        j++;
    else
        k++;

Style C: Linux Style + braces. Much safer than B. Adds one line the entire block over B (will only add one line no matter how many 'else' statements)

    if (test_1) {
        i++;
    } else if (test_2) {
        j++;
    } else {
        k++;
    }

Style D: Conditional statements on new line - Adds one line per conditional statement over B

    if (test_1) {
        i++;
    }
    else if (test_2) {
        j++;
    }
    else {
        k++;
    }

My preference is C - It is a good compromise between code safety and code density. I'm also biased towards C because that is the style I have used the most, so it feels 'natural' to me

[Derakon]

Yeah, my preference from those is D; I just find it easier to read, and C feels needlessly terse.