Pcsx2: Discussion on code formating

[gregory38]

I would like to square the code database to uniformize coding style.

• At a bare-minimum, I would like to remove all trailing white space (small impact on history)
• It would be nice to convert leading tab to space everywhere (big impact on history, depends on the part)
• Reformat with an automatic tool such as astyle or clang-format (huge impact on history)

Potentially we could limit the change to a subset of the database. For example we could start with a couple of plugin onepad/spu2x. I don't know if it possible to add a hook on git to enforce the coding style (aka the hard policy).

edit: hook example can be found in .git/hooks/

Or we could keep current situation huge mess of coding style.

For refernce here all the configuration available on clang-format

Language:        Cpp
# BasedOnStyle:  LLVM
AccessModifierOffset: -2
AlignAfterOpenBracket: true
AlignConsecutiveAssignments: false
AlignEscapedNewlinesLeft: false
AlignOperands:   true
AlignTrailingComments: true
AllowAllParametersOfDeclarationOnNextLine: true
AllowShortBlocksOnASingleLine: false
AllowShortCaseLabelsOnASingleLine: false
AllowShortFunctionsOnASingleLine: All
AllowShortIfStatementsOnASingleLine: false
AllowShortLoopsOnASingleLine: false
AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakBeforeMultilineStrings: false
AlwaysBreakTemplateDeclarations: false
BinPackArguments: true
BinPackParameters: true
BreakBeforeBinaryOperators: None
BreakBeforeBraces: Attach
BreakBeforeTernaryOperators: true
BreakConstructorInitializersBeforeComma: false
ColumnLimit:     80
CommentPragmas:  '^ IWYU pragma:'
ConstructorInitializerAllOnOneLineOrOnePerLine: false
ConstructorInitializerIndentWidth: 4
ContinuationIndentWidth: 4
Cpp11BracedListStyle: true
DerivePointerAlignment: false
DisableFormat:   false
ExperimentalAutoDetectBinPacking: false
ForEachMacros:   [ foreach, Q_FOREACH, BOOST_FOREACH ]
IndentCaseLabels: false
IndentWidth:     2
IndentWrappedFunctionNames: false
KeepEmptyLinesAtTheStartOfBlocks: true
MacroBlockBegin: ''
MacroBlockEnd:   ''
MaxEmptyLinesToKeep: 1
NamespaceIndentation: None
ObjCBlockIndentWidth: 2
ObjCSpaceAfterProperty: false
ObjCSpaceBeforeProtocolList: true
PenaltyBreakBeforeFirstCallParameter: 19
PenaltyBreakComment: 300
PenaltyBreakFirstLessLess: 120
PenaltyBreakString: 1000
PenaltyExcessCharacter: 1000000
PenaltyReturnTypeOnItsOwnLine: 60
PointerAlignment: Right
SpaceAfterCStyleCast: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles:  false
SpacesInContainerLiterals: true
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
Standard:        Cpp11
TabWidth:        8
UseTab:          Never

[gregory38]

Here a basic hook https://github.com/andrewseidl/githook-clang-format/blob/master/clang-format.hook

Reading some discussion on chromium

I recommend using a special "refactor" role account for that, though - to make it easier to spot places where you have to go back an extra step in blame, and so you don't get accused of using this as an underhanded way to get to the top of the leaderboard :)

[avih]

The biggest issue with such global reformatting is that it can make it really _really_ hard to track history. We've had several such refactors in the past (like changing CRLF/LF etc), and it's a PITA.

You can't really use git blame and it just make, IMO, much harder to track code history beyond the refactor commit.

IMO, we should leave it as is, and as new lines of code are committed, make sure they adhere to good standards.

[gregory38]

We got already several big refactor. And you can still seach in the history. So yes git blame wil take you a couple of more seconds, to click on the previous revision on the history. But honestly the time spend in git blame is really small versus the time to review patch/read the code/try to attract new contributors.

There is currently no standard, all sources files have their own standard. No serioulsly it is just a pain. Note: I will also be fine to apply different formating rules to different part of the project (plugins vs core).

So far for me the drawback are

• current PR, local branch will need a rebase.
• natural owner of plugin won't like the new style
• an extra step on git blame/history

However gain are huges

• Higher consistency
• easier to read the code (except maybe in some part microVU that use a clever indentation)
• easier to review the code
• easier to contribute

@ramapcsx2 @sudonim1 @ssakash @FlatOutPS2 @turtleli @refractionpcsx2 @gabest11 voices your opinion.

[refractionpcsx2]

At least it will be standardised i guess? :P

[gregory38]

yes, IMHO, enforcing coding style with tool is the only efficient way to have a coding style.

[avih]

Just make sure this tool is available to and can be used by most contributors (I still don't think we should reformat the whole repo, but obviously I'm just one vote)..

[gregory38]

clang-format is available with the clang toolchain. It is available on linux/windows/OSx. It could be plugged into Visual Studio too.

[ssakash]

FWIW, it would also be nice to have a standard for variable naming and braces placement.

By braces placement, I mean :

if(){
    do_something();
}

VS

if()
{
    do_something();
}

[avih]

clang-format is available with the clang toolchain. It is available on linux/windows/OSx. It could be plugged into Visual Studio too.

This is quite a requirement for contributors, especially on windows.

[turtleli]

Well, it actually seems quite easy to use clang-format on Windows. You can install the clang-format plugin from http://llvm.org/builds/ and a ClangFormat option will appear in Tools menu. And if we add a .clang-format file to the solution directory, clang-format will use that to format the code when the option is used (I tested with Dolphin's .clang-format file).

[gregory38]

I'm glad it is easy to use. And it would likely be easier to decipher coding rule of the current file. Submit, nitpick in review. Rebase. Resubmit...

The global idea is to do a massive reformat (aka the painful change). Then add a script to automatically detect wrong format change on the travis bot.

So it means that clang format won't be mandatory. If people follow the standard. The difference that it would be possible to automatically format stuff. And coding standard will be standard.

[gregory38]

You could find here some info on the various options: http://clang.llvm.org/docs/ClangFormatStyleOptions.html

Some formatting questions

• Do we want to put case in the same indentation level as switch ? Or put an extra indentation ?
• Do we want to limit line length ? If yes, I propose 120. Unfortunately it will split commented code line in several lines.
• Where do we put public/protected/private access modifier? Same as class, in the middle between class (1,2 or 3 spaces) and declarations. At same level as declaration ? (note I put 4 spaces for indentation)

[gregory38]

I pushed a clang-format file setup. I need to play with it a bit but it ought to be close of the final result. I also added a script to check the PRs. However we need to put clang back on travis.

Plan is to convert a couples of "small" (null/cdvd) plugins first then bigger plugins (spu2x/pad). Last step will be GS and PCSX2 (based on the number of PR conflict).

I need to investigate a git hook to automatically format the commit. It will be nice.

[turtleli]

AlignConsecutiveAssignments: true isn't the best.

Example from GSPng.cpp
Before:

for (int y = 0; y < height; ++y, image += pitch) {
    for (int x = 0; x < width; ++x)
        for (int i = 0; i < bytes_per_pixel_out; ++i)
            row[bytes_per_pixel_out * x + i] = image[bytes_per_pixel_in * x + i + offset];
    png_write_row(png_ptr, row);
}

After:

for (int y = 0; y < height; ++y, image += pitch) {
    for (int x = 0; x < width; ++x)
        for (int i                           = 0; i < bytes_per_pixel_out; ++i)
            row[bytes_per_pixel_out * x + i] = image[bytes_per_pixel_in * x + i + offset];
    png_write_row(png_ptr, row);
}

clang can be added back to Travis CI since the apt repo is back up, but I don't think clang 3.8 can compile pcsx2 (name mangling issues). I haven't tried 3.8.1 yet.

[gregory38]

Oh. It sucks. It feels like a bug of the tool.

I will give it a look at clang 3.8

[gregory38]

@turtleli Which version of clang format did you use ? It seems to work as expected on my side ?
I created a PR that manually cast function pointer to void*. It must be good on clang 3.8. I don't know if there is a better solution.

[gregory38]

However it doesn't align properly initializer braces table (like pixel in the same file).

[turtleli]

I used the Visual Studio plugin. I tried the commandline version on Windows just in case the plugin was at fault and it formats it the same way the plugin does (svn r277442, post 3.8.1 I think). EDIT: clang-format on my Linux boot (3.8.1) does the same thing too.

It might be better to avoid vertical alignment though since it'll make some changes bigger than they need to be, and that'll be a pain for reviewing code.

[gregory38]

Well I like vertical alignment but I will disable it.