Code AStyle-ification

Game Talk The Lab
#1

Hi,

I’ve made test-only astyle-ification branch.
I’ve suggested it some time ago here: http://whalesdev.com/forums/index.php?topic=264.msg31937#msg31937
and someone else was also suggesting it here:
http://www.cataclysmdda.com/smf/index.php?topic=306.msg2911

You can take a look at (example) results, here:

Precise command line, that I’ve used was

AStyle.exe -T8 -H -k1 -j -p -w -Y -W1 -m2 -U -c --style=otbs --lineend=windows

(I’ve decided to use lineend=windows, cause well, linux editors handle windows’
newlines much better than windows editors linux’s newlines ;))

I haven’t sent pull request, as this probably will be quite breaking change for anyone…

#2

I dont imagine this would break anything, just make the code more easy to read :stuck_out_tongue:

#3

Can we just go with actual trustworthy characters and use some variant of number-of-spaces instead? I generally disapprove of tabs for the same reason I disapprove of non-breaking spaces - Invisible stuff that you can’t tell what it is without destroying it is not a proper way to write code. It doesn’t help that tab behaviour is inconsistent.

Getting multiple spaces to display as a tab or vice versa is trivial, but I think if we’re going to standardize, we should standardize with something standard and just say “2 space indentation” or something like that. … Partially because that’s what my tab key already does to my code and I don’t want to have to rejigger it. :wink:

More seriously, I fully support running running some style standards, but it might be worthwhile to hop in chat in four or five hours when there’s a good chance of a couple devs hanging around to go over exactly what those standards should be. And then, after getting input, we can randomly commit to one of the more widely accepted models.

I don’t really know what most of your options are short for, so I can’t comment beyond saying I dislike tabs and styles that mix broken and attached brackets (of those, 1tbr is definitely the best though), but honestly having it set up like that is a sight better than the current mishmash, so no real complaints. :stuck_out_tongue:

#4

Hi, this was to push the discussion in ANY direction, as I don’t believe there is anyone who’s content with current state…

I’ll put some of my arguments here.

Regarding tab vs spaces, I’ll repeat stuff from first link:
“tabs for indentation, spaces for formatting”

I’m in a [tab] camp, mainly because you can setup probably any editor, to display it with the width YOU prefer.
(I’m sure about vim, visual studio, code-lite, and I guess code blocks too).

Also in case of some of them, you can actually “display” them (e.g. in vim, you can [tt]:match[/tt] them, in VS you can show them as ‘arrows’, like in text editor, here’s a screenshot http://stackoverflow.com/questions/4065815/how-to-turn-off-showing-whitespace-characters-in-visual-studio-ide)

I don’t think you can do opposite thing easily, that is you have two spaces and display them as four…

and as for tabs, we could actually have pre-commit hooks in git, that would check, that no spaces occur before any printable character…

Regarding brackets, I must say I don’t care, as long as it’s consistent.

However I’m in favor of ALWAYS using brackets even in case of if with a single statement (this is -j option), that is:

if (foo)
{
    bar;
}

and never:

if (foo)
    bar;

so in this case 1TBS simply takes less vertical screen-space, but as I’ve said, I don’t care about it, as long as it’s consistent.

regarding other options and ‘styles’, if you have some time, you can take a look here:
http://astyle.sourceforge.net/astyle.html

P.S. I hope no sane person, would ever vote for something like whitesmith or gnu style ;>)

#5

well I guess it could easily.

Keep in mind there are at least few people doing different stuff based on CDDA, so if
you make that change in main repo, they would probably have problems merging it to their branches…

#6

It might be a headache that’s worth dealing with, though.

#7

I’m for 4-space indentation and allman (–style=allman / --style=ansi / --style=bsd / --style=break / -A1) because 4-space is rather standard and tabs are hidden devils, and allman is more easy to read. I don’t mind variations of allman (brackets for one-liners). I’m not really a fan of 1tbs because it makes the start & end of the code blur with the conditionals / elses and strangles the code, I like it when I have space and can easily identify what this if/else/function is doing and where it starts & ends. 1tbs is really a bitch when the identation is broken too, i.e:

bool method() {
dosomething()
    if(badcode) {
        dosome();
    doyou();
} else if (something() ) { 
    really();
    likethis();
} else 
suprise();
}

All I would see in that code is it’s closing the brackets 3 times.

Allman example

string PrintTheWorld(int index)
{
    if(index >= 0 && index < 10)
        return world[index];
    else
    {
        PrintError("Index out of range for (string)PrintTheWorld(int index)");
        return "";
    }
}

EDIT:

well I guess it could easily.

Keep in mind there are at least few people doing different stuff based on CDDA, so if
you make that change in main repo, they would probably have problems merging it to their branches…[/quote]

I just merged 1-week old CZS with DDA and if you astyled DDA right now, I’dd merge it without any problems. I really don’t think any mod does massive changes to the code, and if they do, they ough to be able to manage their own code, since it’s either an addition or a replace.

PS: kevingarnade and DarklingWolf are on IRC right now, join us.

#8

Allman is terrible. You can see brackets closing three times, and you know where they opened because that’s the whole damn point of indentation!

Indentation your allman method RUINS. I don’t care what bracket it’s connected to, I care what /statement/ I’m ending with that close bracket, and your damn broken brackets decouple the relevant logic!

And NOT putting brackets, randomly, for no real reason I have ever been able to tell but confusing the reader, seems bad form in general. Allman is terrrrrrible.

#9

Line end: unix style
Indent level: 4-space
brackets: Allman, 1tbs, K&R(but why?), or lisp in that order of preference
All clauses should have brackets, I’m ok-ish with dispensing with brackets for conditionals with an inlined statement like:

if( test ) do_stuff();

and possibly other isolated single clause statements, but definitely NOT mixed bracketed and non-bracketed, like:

if(test) {
    stuff1;
    stuff2;
} else
    stuff3;
#10

“we should do allman, except not” :wink:

Can this allstyle thing actually mix and match styles like that? If consistent brackets is important to you, might be best to stick to 1tbs. It’s not my favorite style ever, but it does seem to be “choice 2” for everyone.

#11

[quote=“GlyphGryph, post:10, topic:347”]“we should do allman, except not” :wink:

Can this allstyle thing actually mix and match styles like that? If consistent brackets is important to you, might be best to stick to 1tbs. It’s not my favorite style ever, but it does seem to be “choice 2” for everyone.[/quote]

Don’t be mistaken, I’m saying Allman all the way, there is no broken bracket, indentation isn’t broken, there is no randomness, it makes clean, aired and readable code. It’s just 1tbs with a newline before an opening bracket. But as for putting single statements in brackets, I don’t mind doing that to make someone somewhere happy when reading the code, because like Kevin, that doesn’t really bother me. Not as much as allman vs 1tbs.

Discussions 101: Try not to use “your” in arguments, it’s not “my” allman, it’s not “your” 1tbs, “my” style isn’t shitty, “your” style isn’t retarded, we’re talking about styles, not me or you. Using “your” is the best way to get someone on the defensive and piss them off :slight_smile:

Anyway, tl;dr I vote for Allman (it’s a style, a preference, code won’t run faster with or without), single-statement bracketing is a non-issue for me.

#12

Broken brackets is the technical term referring to a bracket on it’s own line. So yes, your preferred brackets are broken.

Its not inherently a bad thing, and if Kevin wants allman with enforced brackets on multiline statements, I’m cool with that (the lack of such enforcement is my only real problem with allman aside from the quibble about broken brackets). If we can agree on spaces instead of tabs, I’m happy.

And my decision ultimately doesn’t weigh nearly as much as those who are actively contributing to the code.

#13

[quote=“GlyphGryph, post:12, topic:347”]Broken brackets is the technical term referring to a bracket on it’s own line. So yes, your preferred brackets are broken.

Its not inherently a bad thing, and if Kevin wants allman with enforced brackets on multiline statements, I’m cool with that (the lack of such enforcement is my only real problem with allman aside from the quibble about broken brackets). If we can agree on spaces instead of tabs, I’m happy.

And my decision ultimately doesn’t weigh nearly as much as those who are actively contributing to the code.[/quote]

Oh I though broken brackets are like

{ }
Like, broken alignment, my bad.

Anyway, I’m for spaces, I don’t really like the fact that you can’t spot a tab versus a space and the size of the tabs differ depending on the context.

#14

Huh. You may be right, actually… I’ve always heard them discussed as being broken from the line that determines their context, but I could see it being used to mean staggered as well.

I might have to look that up.

Okay, http://astyle.sourceforge.net/astyle.html seems to define it the way I’m used to, since it says “Allman style formatting/indenting uses broken brackets.”

#15

Personally I prefer tabs over spaces because they let the coder view the code the way they want, everyone uses 1 tab indentation so there is no disagreements and the tab key is generally pressed when indenting regardless of if the editor is set to spaces or tabs.

However I’ll take ANYTHING* over the 1 space indentation we have right now…

[sub]*assuming anything is either a tab or a number of spaces in the set {2, 4}[/sub]

#16

I dont really care if its tabs or spaces, but they should have a width of 3-4 for readability.

This would be my personal favorite style.

[CODE]–style=1tbs / --style=otbs / -A10

int Foo(bool isBar)
{
if (isFoo) {
bar();
return 1;
} else {
return 0;
}
}[/CODE]

#17

Here’s a radical notion:

Leave stuff the way it is, even if it’s distasteful, until you change it, in which case you can indent it with however many spaces you like. As long as blocks are consistently spaced, is there any reason it can’t continue to be variable at the project level?

This has the benefit of not breaking code compatibility with existing forks & branches. :slight_smile:

Could this work? Because it’s basically what I do now sometimes.

#18

if you look at the vehicle code it’s all in a different style anyway so that can work. The only reason the lightmap code is 1 space indents was that I was matching whales’ style.

#19

Wow, wasn’t aware discussion went further :smiley:

ok here’s another attempt, that probably will be appreciated a bit more: Allman + 4 spaces:

small piece of https://github.com/gim913/Cataclysm-DDA/blob/feature/astyleification2/mapgen.cpp

    case ot_field:
        for (int i = 0; i < SEEX * 2; i++)
        {
            for (int j = 0; j < SEEY * 2; j++)
            {
                ter(i, j) = grass_or_dirt();
                //------Jovan's-----
                if (one_in(120))
                {
                    if (one_in(30))
                    {
                        ter(i, j) = t_shrub_blueberry;
                    }
                    else
                    {
                        ter(i, j) = t_shrub;
                    }
                }
                else if (one_in(1000))
                {
                    ter(i, j) = t_mutpoppy;
                }
                //------------------
            }
        }

oh small P.S. at some point I was so desperate, that I wanted to make scripts like from_whales.sh to_whales.sh,
to convert source when working on it, and convert it back when committing, but it turned out there are at least
few inconsistencies, that made this impossible…

#20

[quote=“zpmorgan, post:17, topic:347”]Here’s a radical notion:

Leave stuff the way it is, even if it’s distasteful, until you change it, in which case you can indent it with however many spaces you like. As long as blocks are consistently spaced, is there any reason it can’t continue to be variable at the project level?

This has the benefit of not breaking code compatibility with existing forks & branches. :slight_smile:

Could this work? Because it’s basically what I do now sometimes.[/quote]

Regardless of whether we do it all-at-once or incrementally as changes are made*, I’m very in favor of having an official coding style, with the various things clearly decided on (with the caveat that “do whatever you want” is a valid and clear choice). While the current coding style may actually be worse than ANY of our preferred styles individually, I think having different styles in different parts of the project is very nearly as bad as no style at all. In particular I’m very skeptical of people actually sticking to whatever the local style is, in my experience people will flit through the codebase making changes in their own style if they aren’t working in-depth on a particular section. Also it’s far easier to enforce “this is the desired coding style” rather than “the coding style for this file is bla, and the coding style for that other style is bla, but with no broken braces” etc…

*The latter is my preference, I don’t like the idea of having a tens of thousands of lines commit that touches every file in the project and is essentially unreviewable.