Reicast-emulator: Whitespace consistency

[AbandonedCart]

After reviewing the changes for https://github.com/reicast/reicast-emulator/pull/1238 and seeing whitespace issues cited against existing whitespace inconsistencies, my questions is can we pick a specific format, clean up the code, and then follow it?

tabs / 4 spaces?
separated / condensed statements (if(){ vs if () {)?
new line / same line brackets?

---

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

[pold500]

I also propose installing clang format plugin into build tools and writing a format.clang file with the rules, so they would be enforced.

[baka0815]

I personally would indent with tabs.
Regarding brackets I don't have a strong opinion.

[pold500]

I have no strong opinion about what exactly the style should be too, but I have the strong opinion that it should exist at first, and the second that it should be enforced in the project, both when you write code, and in build verification tools, like CI.

I propose:

1. For variable names - underscore style (this_is_variable)
2. if/else/statements -
   if( true )
   {
   }
   else
   {
   //always a brackets even for one statement here
   }
3. spaces - spaces instead tabs, 4 space for a tab.

[AbandonedCart]

@pold500 Android Studio uses camel case variables by default. It's probably best not to create some arbitrary rule that involves needlessly submitting 10,000 lines of edits to "change some names" that git has no concern over, while whitespace issues do cause merge fails.

It also happens to cite "simplifying statements" as a warning and recommends removing unnecessary brackets, if not creating the variable ? true ; false shorthand. This was more a question of what should be the general rule in terms of spacing. I don't think restricting the entire design of the statement to one format is rational.

[baka0815]

I would prefer
C if (condition) { <tab>something } else { <tab>something }

Use brackets whenever there is more than one line in at least one block. If all cases consist of just one line then no brackets.

Regarding variable names I'm fine with either variable_name (snake case) or variableName (canel case).

Using tabs for indentation has the advantage that every developer can set the tab spacing in their IDE to their likings (2 or 4 spaces wide).

[AbandonedCart]

@baka0815 Seems reasonable. That is something both Xcode and Android Studio could handle (which are my primary concern because Windows / Linux are pretty adaptable)

[skmp]

I have a stylesheet somewhere from the nulldc/e days.

in general, 4-spaces ident, and the C# styling guide.

[skmp]

Ideally, we'd use clang-format and also have validation for it - it has worked great for other projects.

[pold500]

Here's an example of .clang-format file.

We can discuss every rule, how we want it to be.

AccessModifierOffset: -4
AlignAfterOpenBracket: true
AlignConsecutiveAssignments: false
AlignConsecutiveDeclarations: false
AlignEscapedNewlines: Left
AlignOperands: false
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: false
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: InlineOnly
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: true
BinPackArguments: false
BinPackParameters: false
BreakBeforeBinaryOperators: None
BreakBeforeBraces: Stroustrup
BreakBeforeInheritanceComma: true
BreakBeforeTernaryOperators: true
BreakConstructorInitializers: BeforeComma
BreakStringLiterals: true
ColumnLimit: 100
CommentPragmas: ''
CompactNamespaces: false
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 2
ContinuationIndentWidth: 2
Cpp11BracedListStyle: true
DerivePointerAlignment: false
DisableFormat: false
FixNamespaceComments: true
ForEachMacros: []
IndentCaseLabels: false
IncludeCategories:
  - Regex: '^("|<)stdafx\.h(pp)?("|>)'
    Priority: -1
  - Regex: '^<(W|w)indows.h>'
    Priority: 1
  - Regex: '^<'
    Priority: 2
  - Regex: '.*'
    Priority: 3
IncludeIsMainRegex: '(_test|_win|_linux|_mac|_ios|_osx|_null)?$'
IndentCaseLabels: false
IndentWidth: 4
IndentWrappedFunctionNames: false
KeepEmptyLinesAtTheStartOfBlocks: false
MacroBlockBegin: ''
MacroBlockEnd: ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
PenaltyBreakAssignment: 0
PenaltyBreakBeforeFirstCallParameter: 1
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 9999999
PointerAlignment: Left
ReflowComments: true
SortIncludes: false
SortUsingDeclarations: true
SpaceAfterCStyleCast: false
SpaceAfterTemplateKeyword: true
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: false
SpacesInCStyleCastParentheses: false
SpacesInContainerLiterals: true
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard: Cpp11
TabWidth: 4
UseTab: Never
---
Language: Cpp
---
Language: ObjC
ObjCBlockIndentWidth: 4
ObjCSpaceAfterProperty: true
ObjCSpaceBeforeProtocolList: false
---
Language: Java
BasedOnStyle: Google
BreakAfterJavaFieldAnnotations: true

[AbandonedCart]

@pold500 That's a bit ridiculous. We're not writing the SOP for Microsoft. Get too strict and you are going to scare off any sort of contribution.

[pold500]

@LoungeKatt first of all, this just an example. I'm not saying we should inspect every possible rule. But you can look at "Spaces*" parameters, for example, and make a clang-format file containing only them, for the start. Then we can continue adding new rules on per need basis.

For example, you can start reviewing our policy on
SpacesInContainerLiterals
SpacesInParentheses
SpacesInSquareBrackets

So nothing ridiculous, especially when you press ctrl+k,f and see your code magically format to what is right.

[skmp]

Do you have some examples on how formatting looks like with that style-sheet? To agree with @LoungeKatt, I think we need to only require this for merging to master, and not to say for dev branches.

Formatting can also be done as a pre-commit hook in most dev environments.

Also, as @LoungeKatt mentioned we need to see how we'll apply the initial formatting change. Is there some way for github to 'disregard' some commits for the stats?

[AbandonedCart]

@pold500 Ctrl+k? I don't even have the buttons for that...

[baka0815]

As seen in #1257 we should also remove trailing whitespaces.

[skmp]

It really sounds like we want the clang-format thingy

[skmp]

Anyone up for iterating on @pold500's config and discover a sensible one that has minimal changes to the source?

[AbandonedCart]

Should probably do it for core exclusively for now. Let the default IDEs handle proprietary code for shells.

[skmp]

Agreed there

[baka0815]

I created a .clang-format file under core (see #1311) with basic rules.

[baka0815]

How do we want to format consecutive assignments?
https://github.com/reicast/reicast-emulator/pull/1338/commits/bd6b0c003d94a14c0dda74ef6f2fd7ac645f5441 suggests we want to do
cpp x = 123; next = "test";
however I would prefer
cpp x = 123; next = "test";

The latter helps searching for assignments (you can just search for "x =").

[AbandonedCart]

I think you are using bd6b0c0 as a segue for unrelated discussion. The commit message states the self-contained nature of the formatting specifically to avoid it being exploited for something like this. It is a list of configuration options that are written to a table, and thus formatted as a table in the source.

[baka0815]

Sorry, I didn't want to criticize you or something.
However if we would implement a clang formatting rule, we might need to choose how to format (or ignore) consecutive assignments. And as I saw you reformatting the code I thought it would be a good opportunity to talk about that.

[AbandonedCart]

I'm sure even clang rules support exclusions. I was pointing out that the specific section you are citing is not meant to reflect the universal style. It has been formatted that way since day 1, but some of the options that have been added along the way didn't always follow it.