This is still WIP, I'm mostly publishing this because I want your opinion. To read more about clang-format, see https://clang.llvm.org/docs/ClangFormat.html
Is this something we want?
While testing this, I noticed that the source code is very inconsistent in style. This could help (in addition to potentially making reviews a little bit less about style issues). Perhaps we could even use clang-format as a linter for patches:
Details
Details
- Reviewers
Krinkle
-
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint OK - Unit
No Unit Test Coverage - Build Status
Buildable 12847 Build 25149: Vulcan Build Jenkins Build 25148: Vulcan Build (macOS) Jenkins Build 25147: Vulcan Build (Windows) Jenkins Build 25145: arc lint + arc unit
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/697/display/redirect
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1213/display/redirect
Comment Actions
This works on all platforms where you can use clang-format, which I assume is Windows as well (Visual Studio might even have integrated support).
Comment Actions
Here is a working version using Coala (not enabled in .coafile here).
It is not yet ready to deploy. Indeed, whereas clang-format does a great job at proposing consistent style conventions, and is highly configurable, it has a limitation that I think is unacceptable.
clang-format has no option to accept spaces around binary operators. From Stack Overflow:
z = sqrt(x*x + y*y);
is reformatted as
z = sqrt(x * x + y * y);
which I believe is incorrect. Grouping some terms is very important to understand the code better, especially in mathematic formulae. Moreover, this would require us to change a huge part of our code, whereas we should follow our usual style and enforce its consistency.
There is no way to deactivate this enforcement. So I think we shouldn't use clang-format right now.
Maybe we could try to make upstream add the feature; or I could try to tweak the coala bear to discard this specific change.
In the meantime, I am proposing in D2490 a less strict linting using Artistic Style.
Comment Actions
@wraitii, @vladislavbelov, what do you think about the spaces-around-operators issue? If you agree with me, I will move forward with D2490 (see some discussion there) and try to find a way to fix this one issue with clang-format. If you both think spaces everywhere are better, I can activate this instead.
Comment Actions
I don't have a very strong opinion overall, but starting with a more lazy approach seems fine to me.