Angband.oook.cz
Angband.oook.cz
AboutVariantsLadderForumCompetitionComicScreenshotsFunniesLinks

Go Back   Angband Forums > Angband > Development

Reply
 
Thread Tools Display Modes
Old April 22, 2016, 04:21   #1
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Angband Coding Style

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:
Code:
            /* 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:

Code:
            /* 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:

Code:
         foo(buf,
             sizeof(buf),
             FLAG_UNUSED,
             FLAG_TIMED,
             FLAG_DEAD);
Over this (which is the current Angband coding style):

Code:
         foo(buf, sizeof(buf), FLAG_UNUSED,
              FLAG_TIMED, FLAG_DEAD);
Partly because if we add another parameter, we end up with:

Code:
         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:

Code:
         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:

Code:
        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:

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

Code:
        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
calris is offline   Reply With Quote
Old April 22, 2016, 05:03   #2
Derakon
Prophet
 
Derakon's Avatar
 
Join Date: Dec 2009
Posts: 8,828
Derakon is on a distinguished road
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.
Derakon is offline   Reply With Quote
Old April 22, 2016, 05:14   #3
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by Derakon View Post
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 is offline   Reply With Quote
Old April 22, 2016, 09:56   #4
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
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.
calris is offline   Reply With Quote
Old April 22, 2016, 12:42   #5
PowerWyrm
Prophet
 
PowerWyrm's Avatar
 
Join Date: Apr 2008
Posts: 2,623
PowerWyrm is on a distinguished road
Quote:
Originally Posted by Derakon View Post
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...
__________________
PWMAngband variant maintainer - check http://powerwyrm.monsite-orange.fr (or http://www.mangband.org/forum/viewforum.php?f=9) to learn more about this new variant!
PowerWyrm is offline   Reply With Quote
Old April 22, 2016, 13:00   #6
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by PowerWyrm View Post
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...

Last edited by calris; April 22, 2016 at 13:35.
calris is offline   Reply With Quote
Old April 22, 2016, 13:14   #7
takkaria
Veteran
 
takkaria's Avatar
 
Join Date: Apr 2007
Posts: 1,930
Donated: $40
takkaria is on a distinguished road
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.
__________________
takkaria whispers something about options. -more-
takkaria is offline   Reply With Quote
Old April 22, 2016, 13:33   #8
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by takkaria View Post
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.

Quote:
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
calris is offline   Reply With Quote
Old April 23, 2016, 06:19   #9
molybdenum
Apprentice
 
Join Date: May 2013
Posts: 84
molybdenum is on a distinguished road
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:

Code:
if (x) {
    ...
} else if (y) {
    ...
} else {
    ...
}
mainly because it collapses down to something like this:

Code:
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:

Code:
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.
molybdenum is offline   Reply With Quote
Old April 23, 2016, 08:54   #10
calris
Adept
 
Join Date: Mar 2016
Posts: 194
calris is on a distinguished road
Quote:
Originally Posted by molybdenum View Post
mainly because it collapses down to something like this:

Code:
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'?

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

Code:
if (x) {}
else if (y) {}
else {}
Putting code on the same line as the condition is completely wrong. You end up with

Code:
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.

Quote:
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
calris is offline   Reply With Quote
Reply


Currently Active Users Viewing This Thread: 1 (0 members and 1 guests)
 
Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

BB code is On
Smilies are On
[IMG] code is On
HTML code is Off

Forum Jump

Similar Threads
Thread Thread Starter Forum Replies Last Post
Sil: 'forge' key in Angband-style keyset? Therem Harth Sil 3 October 24, 2012 17:29
Skyrim, angband style BlackKite Idle chatter 8 June 27, 2012 02:28
some more questions about coding Malak Darkhunter Vanilla 16 January 3, 2012 04:52
more variant coding woes will_asher Variants 1 February 10, 2011 05:18
Angband coding question will_asher Variants 10 September 3, 2008 22:03


All times are GMT +1. The time now is 08:09.


Powered by vBulletin® Version 3.8.11
Copyright ©2000 - 2019, vBulletin Solutions Inc.