WIP: Format code using clang-format

[brakhane]

I find having a common code style in projects where multiple people work on can be an advantage in grokking the code, as the code is consistent throughout.

So I propose to use clang-format, this has the advantage that VS has built in support for it, so you can just use "format document" to automatically update the current file according to the guidelines.

This PR is my attempt to introduce clang-format rules that are compatible with the code style you are currently using. I think I've succeeded in mapping most of your choices, except for one, see later.

It's intended as a basis for discussion, not as something to merge right away.

The PR consists of 3 commits, I recommended to look at them seperately, otherwise it's overwhelming.

use macro to set/unset flags

This commit changes wiScene_Component.h to use a macro for the common pattern if (value) _flags |= FLAG else _flags &= ~FLAG . This makes it both easier to spot when that pattern is not used in some implementations, and also prevents clang-format to split some of those functions into multiple lines later on

Introduce clang-format

This commit actually creates the clang-format settings, and also configures exceptions:

• Everything in Utility is excluded, as that is mostly vendored code
• Same for Jolt, LUA, and some subdirectories of shaders
• sets //clang-format off in vendored in files that are in other directories
• Updates BIN2H to automatically add //clang-format off in generated files

For the actual formatting rules see later

format all code with clang-format

This changes all files according to the configured style. I recommend using Split view for the diff, as the unified diff gets confused easily and can make the changes incomprehensible and seem more drastic than they are. Alternatively, checking out the commit or looking at it directly in Github is also a good idea.

Changes/Inconsistencies

I've tried hard to find a preset that causes the least amount of changes. There are a few spots where I had to make a guess what your preferred style is, as you were using different ones in different files. I generally chose the one that caused the least changes over all files.

What changed

indentation of lambdas

There is one pattern that clang doesn't support, and that has to do with lambdas used as callbacks:

Most of the time, when there is a callback, the code looks something like this

foo([](bar, baz) {
    ... some lambda code ...
    ... more code ...
    });

that is, the closing bracket is indented on the same level as the lambda body. Clang insists to change this to

foo([](bar, baz) {
    ... some lambda code ...
    ... more code ...
});

arguably, that's more consistent compared to other functions, but it is a change to how most of the code was indented. (There were instances where the "clang pattern" was already used, but those were fewer than the unindented ones)

The good news is that that is basically the only style decision I couldn't get clang-format to replicate. In other cases, there were multiple styles in use, and I had to choose one:

Braced list style

there were 2 styles for c++ braced lists in use:

Foo foo = {bar, baz} // no spaces
Foo foo = { bar, baz } // spaces

the majority of cases were with spaces, so I chose that; that means all braced lists will now have spaces, but it can easily changed to be the other way round

Alignment of parameters of function calls

I've seen both variants:

    foo(param1,
        param2,
        param3);  // parenthesis on same line

    foo(param1,
        param2,
        param3
    ); // parenthesis on own line

the first variant seems to be more common, so I went with that

Aligned types

float foo;
int   bar;  //aligned

float foo;
int bar; //unaligned

both were used, unaligned was much more common

indented namespaces

namespace foo {
    code
} // 1

namespace foo {
code
} //2

Although I prefer 2 (see later), 1 was much more common, so it was chosen. wiRenderer.cpp is one of the counter examples that therefore has a seemingly huge diff.

space after template

template<typename x> was more common than template <typename x>, so the former it is

pointer and reference "alignment"

int* a;
int *a;
int& a;
int &a;

Both pointer and reference on the left were the most common, so that's what's used here.

blocks in case statements

case 1:
    foo();
    bar();
case 2: 
{
    block1();
    block2();
}
case 3:
    {
        block1();
        block2();
    }

When blocks with braces appear in case statements, the braces can either be at the same height of the case, or indented. The first variant (case 2) was more common than the second.

My suggestions to tune the style

The following is a list of things I think we could change compared to the current style that will make it a bit nicer; but that's obviously subjective:

comment namespaces and don't indent them

clang-format can automatically mark the end of namespaces with a comment, this would allow us to get rid of the indentation for namespaces, imo. This has the advantage that the code is not so much indented like it is in current files. Example:

namespace wi {
namespace foobar {
... code for wi::foobar
} // namespace wi::foobar
} // namespace wi

Enforce a space after //

Currently, some comments, mostly commented out code, do not have a space after //. Since clang-format can automatically add those, I think it's a good idea to let it change //#define ... to // #define ....

sort imports

clang-format allows to sort the imports, which I usually find to be a good idea, it makes it easy to see what files are included, and prevents cases where the import order matters.

Currently, there seems to be a case where reordering the imports breaks the build, therefore I haven't activated it. What currently is configured, but disabled, is something like the following:

#include "stdafx.h" // if it's used, it's always on top
#include "foo.h" // if we're in foo.cpp, it's own header comes next
#include <bar> // system headers are next
#include "wiFoo.h" // followed by all wiXXX headers
#include "baz.h" // and then everything else

Do you think such an order would make sense? Or would another order be better? Or should we not sort files at all?

Summary

I think adding clang-format will help making the code look more consistent and help with grokking it. Let me know what you think.

[turanszkij]

I tried this in the past but it caused a big slowdown for code editing, and other annoyances, like overriding my manual indentations at specific places.

I'm willing to test it again, but can you make the PR not change all the code files, because it can't be easily reviewed when there are hundreds of changes and even changes third party code. I think this PR should only include the clang format file itself and nothing else.

[brakhane]

You can just look at bae5ba4, it only has the proposed config.

[turanszkij]

I'll try it sometime but that commit still has a lot of other changes. It should only work for the lines anybody modifies in the future, it shouldn't reformat a lot of existing code, especially third party files.

[brakhane]

It should only work for the lines anybody modifies in the future, it shouldn't reformat a lot of existing code, especially third party files.

The only other changes in that commit are additional clang-format files to prevent it from formatting third party files, as well as adding "clang-format off" to some generated files that are in directories that usually contain non-vendored code, but shouldn't get formatted regardless. And there is one change to Bin2H to also add that "disable clang-format" comment to all files it creates.

I don't think it's possible (at least in any reasonable fashion) to have clang-format only apply to changed files. For example, if it's decided that namespaces should be indented, wiRenderer currently does violate that. If clang-format would then only format the changed lines, suddenly all changed lines will be indented by additional 4 spaces, while unmodified lines won't, which would look ugly and make the code very hard to read.

In my experience, if a formatter is introduced in a project, the least painful way is to bite the bullet and have one commit that reformats all files once. I agree it's far from ideal, but usually all other options are worse.

All that being said, my main goal for this discussion/PR is to figure out some sort of style guide first. Once we figure out some clang-format rules you're ok with (or some other formatter), I can think of a way to introduce those changes in the least painful way.

[VinnyVicious]

I'll try it sometime but that commit still has a lot of other changes. It should only work for the lines anybody modifies in the future, it shouldn't reformat a lot of existing code, especially third party files.

I agree with this. It is a nightmare to understand why something has changed, or trying to pin point a change that introduced a bug, and not being able to do so because of a code style mega commit.

[brakhane]

@VinnyVicious It's a lot easier to exclude one commit from bisect/diff whatever than you think. Also, as I've said, it's not feasible to adjust code style changes only in changed lines:

1. It looks ugly and very inconsistent. At this point, you don't need to have a code style at all
2. In cases where the new style causes a file to be indented differently, you'll end up with a mess. Consider the following:

namespace foo {
    void bar() {
        if (baz) {
            do_this();
            and_this();
            and_that();
        }
    }
}

Let's say the new style calls for namespaces to not be indented anymore, and also that we needed to change the if condition as well as the second line in that, you'll end up with code like

namespace foo {
    void bar() {
    if (baz || baz2) {
            do_this();
        and_something_else();
            and_that();
        }
    }
}

you could try to work around that by saying that if you touch a method, you'll have to apply the code style to the whole function, but then you'll still end up with some methods indented 4 spaces while others are not, also, it makes it even harder in a commit to see what actually changed.

The only thing you can do that makes it kinda bearable is having a rule to change the whole file once you make a change, but even that leads to a lot more noise. In my experience, just biting the bullet and having a short code freeze, applying all the code style changes in one commit that is clearly labeled as such (and that does do nothing else) is the least headache inducing.

eg. git blame has a --ignore-rev parameter, and you can even give git a list of revisions to completely ignore for other commands as well, see this for an example

[turanszkij]

I added clang-format PR here: #937 (using the clang format file from @brakhane ). This doesn't change existing files, but new code can be formatted with the clang format file. Also it doesn't have multiple clang format files, just one in the root directory. The formatting will only take place when writing code (at least that's how it works in Visual Studio). It will not format code when adding files to the project (for example adding a library) and doesn't modify old code.

Auto formatting everything that existed before is unacceptable to me, and I also want to be able to override formatting on a case-by case basis, for example manually tabbing a block of code to be aligned for easier column editing, like here for example:

Formatting is done in Visual Studio when I enter }, ; or Ctrl+K+D. If not using these, manual formatting with tabs, etc. is doable easily.

[brakhane]

Also it doesn't have multiple clang format files, just one in the root directory.

Be aware that this will cause the format rules to also get applied to files in third party directories. That's all what the other clang-format files were there to prevent.

Auto formatting everything that existed before is unacceptable to me

It's your decision and I will respect that. I'm still convinced it will cause annoyances down the road, like small changes suddenly modifying the whole file because it wasn't touched before, which also breaks git blame (you can tell git blame to ignore certain commits, but when the formatting happens alongside genuine changes in the same commit, you lose that ability). It also makes reviewing old commits more time intensive if they contain a lot of formatting changes; it's easy to miss a subtle change that was introduced.

and I also want to be able to override formatting on a case-by case basis, for example manually tabbing a block of code to be aligned for easier column editing

that's a common use case, formatting can be temporarily disabled by // clang-format off and re-enabled with // clang-format on after the problematic blog of code.

[turanszkij]

But it didn't modify whole files in the linked pull request, it only modifies lines that I am currently editing. Or multiple lines if the } or ; is entered which closes a multiline scope. Formatting it can also be quickly cancelled with Ctrl+Z (undo). And if I add a new file but not modify it, no formatting changes will be made.