Cxbx-reloaded: Re-indent all our code

[PatrickvL]

Instead of everyone applying their own indenting style to Cxbx-Reloaded code via relatively small pull requests, update our entire code base to one style, in one go, without any other change, and if possible make it so that future code automatically adheres to this style

[x1nixmzeng]

We could hook up https://clang.llvm.org/docs/ClangFormat.html - but it'd be worth defining some style guidelines first.

The downside is any third-party libraries we've imported into the repro (rather than moduled).

[PatrickvL]

True. Those would have to be excluded from automatic reformatting (except OpenXDK perhaps, since that's been modified so much, it's hardly the same thing anymore, plus, the project itself is in hibernation)

[RadWolfie]

Currently, git doesn't work with indenting our code before it is pushed into repo. It seems to require some sort of extension in order to have filter option working. Only new lines are working.

I would say Clang format extension is a requirement plus AppVeyor has support for this? Or would it be TravisCI?

[x1nixmzeng]

You can setup commit hooks, which run local scripts before committing.

Personally, I wouldn't bother enforcing standards on a build machine - having an automated build fail because of style is a waste of time in my opinion :stuck_out_tongue:

[PatrickvL]

Keeping the style consistent first and foremost requires a choice for an actual style.
Automation is of later concern.

Can we list and discuss a few smart styles here?

[RadWolfie]

@x1nixmzeng Not true, it will tells us the contributor isn't following our standard which need to be fix before it can go into master. 😉

Currently, both source code and github repo are showing both spaces and tabs making it look out of order which is not a good design. Using Clang format enforcement will help smooth the workflow better into main branch. I want to mention this, @LukeUsher did state Clang format extension is cross-platform supportive.

I think we need to change the topic to "format standard" with listing in op post.

[PatrickvL]

Let's keep it simple. First define a style. Then apply it to all our code. Then document it. And finally evaluate cost versus benefit of automation.

[RadWolfie]

We can create .editorconfig file with the following code below to force fix github's viewing issue between tab and spaces mixture problem.

[*]
indent_size = 4

I do not understand why github is using 8 spaces for a single tab.

.editorconfig file extension is supported without plugin requirement by the way.

Then we can slap in Clang format extension file to meet our standard format since there's no documentation state it is support without plugin requirement for github. We can leave .editorconfig file in github permanently.

[slx7R4GDZM]

I do not understand why github is using 8 spaces for a single tab.

The standard for tabs was originally 8 spaces until recently. I think it was this way on vi and even typewriters.

[RadWolfie]

Thanks for the explanation @slx7R4GDZM! 😃

[RadWolfie]

Ident style we should be using is OTBS variant under K&R.

Under ideas for .editorconfig, indent_brace_style does not seem to apply at all with Visual Studio 2017.

Also, I notice Clang format is integrated with Visual Studio 2017. See below for example.

[PatrickvL]

From that same Wikipedia page, the Stroustrup style most closely describes what we're using right now (if you'd ignore tabs and indent size)

[RadWolfie]

Here's a mockup of clang format I had made. I can't find @LukeUsher's clang format file from pm months ago to compare against my code. The mockup will help enforce things better into standard coding with some flexibility of course.

AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: 'true'
AlignConsecutiveDeclarations: 'true'
AlignEscapedNewlinesLeft: 'true'
AlignOperands: 'true'
AlignTrailingComments: 'true'
AllowShortCaseLabelsOnASingleLine: 'false'
AllowShortFunctionsOnASingleLine: None
AllowShortIfStatementsOnASingleLine: 'false'
AllowShortLoopsOnASingleLine: 'false'
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: 'false'
AlwaysBreakTemplateDeclarations: 'true'
BinPackParameters: 'true'
BreakBeforeBraces: Stroustrup
IndentCaseLabels: 'true'
Language: Cpp
MaxEmptyLinesToKeep: '2'
NamespaceIndentation: All
SpaceBeforeAssignmentOperators: 'true'
SpaceBeforeParens: Never
SpaceInEmptyParentheses: 'false'
SpacesInAngles: 'false'
SpacesInCStyleCastParentheses: 'false'
SpacesInContainerLiterals: 'false'
SpacesInParentheses: 'false'
SpacesInSquareBrackets: 'false'
...

The reason to use the first code below is to show the chain in human readability. Only time to a have new line after if/else is another if statement. Which automatic recognize as "will check statement again", for some people like me to read and understand quickly.

Human readability, not exactly able to expand and collapse as it overlaps. (I believe this is very old IDE bug.)

if (test == 0) {
  test = 1;
} else if (test == 4) {
  test = 2;
} else {
  test = 0;
}
if (test== 0) {
  test = 10;
}

vs

Not really human readability, yet still able to expand and collapse just fine.

// 
if (test == 0) {
  test = 1;
}
else if (test == 4) {
  test = 2;
}
else {
  test = 0;
}
if (test== 0) {
  test = 10;
}

vs

Not very human readability plus waste of screen spaces.

if (test == 0)
{
  test = 1;
}
else if (test == 4)
{
  test = 2;
}
else
{
  test = 0;
}
if (test== 0)
{
  test = 10;
}

[ghost]

Human readability is a subjective issue IMO. For example, the second case is much more readable to me than the first one.

[PatrickvL]

I've found it helpful, pretty and descriptive to put empty lines between compound statement blocks in the same indent level. So in the above example, the empty line would go before the last if.

[RadWolfie]

Okay so only this line will need to change: BreakBeforeBraces: Linux to BreakBeforeBraces: Stroustrup. And I don't think there is a format to make if statement as new line. We'll have to monitor each other for that. Unless I have overlooked in the documentation.

EDIT: Thanks for the example. Or else I'll give you a "huh?" expression. 😛

[Fisherman166]

@RadWolfie What happened to this? I'd love to see this get into production as the codebase is all over the place right now.

[RadWolfie]

Each of us has various of major re-work in forked and locally, except most of them doesn't work while still has some improvement. Much as we want to put this into production, it will affect a lot of source codes while trying to make sense of what's been changed in each pull request.

This is part of a reason it's been delayed for long time and still has been.

[PatrickvL]

In my opinion old branches shouldn't hinder the codebase going forward. Having files moved, renamed and re-indented goes a long way towards reaching a better maintainable codebase, which would be better accessible for newcomers, make it easier to work on new features, and thus be generally beneficial for everyone.
Old branches can still be compared against their even older parent to see which parts have changed.