# Clang format discussion
**URL:** https://discourse.vtk.org/t/clang-format-discussion/2073
**Category:** Development
**Tags:** proposal
**Created:** [November 8, 2019, 1:10pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073 "2019-11-08T13:10:00Z")
**Posts on this page:** 20
**Page:** 1
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 8, 2019, 1:10pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/1 "2019-11-08T13:10:00Z")
At the hackathon this week, there was discussion of using `clang-format` on VTK to enforce coding style guidelines. This allows formatting of a merge request using `Do: reformat` so that local formatting isn’t necessary (and that various versions of `clang-format` don’t keep disagreeing on minor details in the formatting). ParaView, CMake, VTK-m, and the SMTK projects have been doing this for quite a while with great success.
However, to do this effectively, a sweep of the entire codebase to normalize the formatting is required (otherwise any edit to a file is going to force a reformat of the entire file leading to very confounding MR reviews). It is best to do this _before_ the 9.0 release so that backports can actually be applied between the `release` and `master` branches with the same code.
For those wanting a way to easily rebase your branches across the reformat, a procedure will be provided (I know how it should work, but need to write down the steps and test them out for robustness). Any incremental (e.g., directory-at-a-time) reformatting would require this process for each formatting “sweep” your merge requests touch. Due to this reason, a single reformatting sweep is preferred.
By using [whatstyle](https://github.com/mikr/whatstyle) (thanks @seanm!) on ParaView (which has consistent style), it has determined that ParaView’s `clang-format-8` configuration file based on the contents of `ParaViewCore/ClientServerCore/Core` with some hand-tuning for parameters that `whatstyle` can’t figure out on its own (namely `ColumnLimit`):
```auto
BasedOnStyle: Mozilla
AlignAfterOpenBracket: DontAlign
AlignOperands: false
BinPackArguments: true
BinPackParameters: true
BreakBeforeBraces: Allman
ColumnLimit: 100
Standard: Cpp03
```
Which is very similar to what is used today.
An initial MR to sort include directives is [here](https://gitlab.kitware.com/vtk/vtk/merge_requests/6166). This is done separately because it is highly likely to change behavior compared to the rest of the formatting configuration parameters. I’ll work through its failures until the dashboards are happy, though I suspect the nightly builders may have some followup thoughts on the sorting as well.
Cc: @utkarshayachit @Dave_DeMarle @RobertMaynard @brad.king @seanm @dgobbi @mwestphal
---
### Author:  [@Dave\_DeMarle](https://discourse.vtk.org/u/Dave_DeMarle)
#### Post date: [November 8, 2019, 2:30pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/2 "2019-11-08T14:30:45Z")
I think this will be great.
Removing the pedantic formatting barrier-to-entry for new developers is worth the git blame history wall in my opinion. There is always git log --follow to peer behind the wall when we need to.
---
### Author:  [@brad.king](https://discourse.vtk.org/u/brad.king)
#### Post date: [November 8, 2019, 3:26pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/3 "2019-11-08T15:26:30Z")
For reference, CMake and ParaView transitions both left empty commits just before the transition commit that provides instructions for rebasing work-in-progress topics across the transition. See [CMake’s commit](https://gitlab.kitware.com/cmake/cmake/commit/12fed3edb107c949671043196fa94c542b45452a) and [ParaView’s commit](https://gitlab.kitware.com/paraview/paraview/commit/609f2afcd5e2f963e4fe36203b08d2ebfc0476f9).
---
### Author:  [@amaclean](https://discourse.vtk.org/u/amaclean)
#### Post date: [November 8, 2019, 11:14pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/4 "2019-11-08T23:14:19Z")
This is a really good idea.
FYI, ages ago, I dropped a .clang-format file into VTKExamples/src/Cxx which (I think) was based on the CMake one. It works quite well in both KDevelop and Visual Studio. Have a look at it if you are interested.
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 9, 2019, 5:00am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/5 "2019-11-09T05:00:36Z")
The point here is that it doesn’t matter what clang-format version developers have laying around or whether their editors support it. It also does not apply to third party code (which we should not be reformatting; the robot also checks for “improper” changes to third party code as well). The robot will check all MRs for proper formatting and reject them if they don’t adhere to what we’re using. It can, however, also _fix_ your MR automatically via a `Do: reformat` command.
---
### Author:  [@amaclean](https://discourse.vtk.org/u/amaclean)
#### Post date: [November 9, 2019, 7:04am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/6 "2019-11-09T07:04:10Z")
I like it! This willl be so useful.
Andrew Maclean
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 14, 2019, 2:59pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/7 "2019-11-14T14:59:22Z")
The header sorting has been merged. I’ll try and get the full reformat up by the end of the week. An initial run had a 2.5M line patch for anyone curious.
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 14, 2019, 3:58pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/8 "2019-11-14T15:58:54Z")
The branch is available [here](https://gitlab.kitware.com/ben.boeckel/vtk/commits/clang-format). I need to write the docs and inject a commit with instructions for handling rebasing over the commit yet.
---
### Author:  [@simonesneault](https://discourse.vtk.org/u/simonesneault)
#### Post date: [November 14, 2019, 4:17pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/9 "2019-11-14T16:17:16Z")
Hello,
Thanks for that effort, it will make the code/PR much more readable !
About the includes, some class uses either “” or \<\> and there are some space between some group of includes ([vtkOpenGLGPUVolumeRayCastMapper](https://gitlab.kitware.com/vtk/vtk/blob/master/Rendering/VolumeOpenGL2/vtkOpenGLGPUVolumeRayCastMapper.cxx) is a good example), will clang format handle that properly ?
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 14, 2019, 4:47pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/10 "2019-11-14T16:47:32Z")
It sorts within groups. I don’t think it knows to make sure certain headers use `""` or `<>`. There is support via `IncludeCategories` ([https://clang.llvm.org/docs/ClangFormatStyleOptions.html](https://clang.llvm.org/docs/ClangFormatStyleOptions.html)) for doing that logic. CMake just moved over to using it.
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 15, 2019, 2:59pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/11 "2019-11-15T14:59:15Z")
I’d like to merge the reformat later today. I’ll have a preview of the branch up in a merge request around noon after I’ve tested that things still build on my machine. At the end of the day (around 4:30 or so), I’ll do a final rebase, run the build locally again, and let the robot sanity check it and then merge it. I don’t know that the buildbots will get back fast enough to not have something else go in before it and conflict with it (plus, if we have tests dependent on the formatting of the code…).
Any objections? @Dave_DeMarle @ken-martin @allison.vacanti @seanm (sorry, this will inevitably make const madness need yet another rebase) @utkarshayachit @brad.king @RobertMaynard
---
### Author:  [@brad.king](https://discourse.vtk.org/u/brad.king)
#### Post date: [November 15, 2019, 3:03pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/12 "2019-11-15T15:03:03Z")
Fine with me. I’d like to go over the branch together IRL when you prepare it.
---
### Author:  [@estan](https://discourse.vtk.org/u/estan)
#### Post date: [November 15, 2019, 4:43pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/13 "2019-11-15T16:43:07Z")
This sounds like a nice change. We have something similar at work, but where the build will fail if it doesn’t adhere to the clang-format style we use. Having a reformatter bot would be even nicer, though we’ve still saved a lot of time during reviews.
The only thing I can recommend is picking the very latest clang-format version you can get your hands on for the bot. That way its longevity in terms of staying packaged in distros, being supported upstream etc will be maximized.
We are in the situation now that we set up our CI format check back when clang-format-4.0 was recent, and have recommended our devs to auto-format on save using that version, to avoid a yelling CI bot, but now clang-format-4.0 is ancient and we’re considering bumping the version on our CI, which will bring with it some behavioral changes (even given the same config).
So just a recommendation to go with the latest, to postpone that day for you as much as possible. Our situation is a little different of course, and you guys may be able to run the same version on the bot for eternity.
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 15, 2019, 6:23pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/14 "2019-11-15T18:23:55Z")
I have a pending change into the robot to be able to just do the sweeping reformat, so updating in the future will be as easy as:
- make new version available on the bot
- prep a topic which changes `format.clang-format=8` to `=9`
- make an empty commit for the robot to reformat the repo into
- run the reformatter until it hits a fix point
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 15, 2019, 8:07pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/15 "2019-11-15T20:07:59Z")
So a wrench in the process. HeaderTests had some failures. Looking into it, a lot of macro calls do not have a trailing semicolon. `clang-format` goes a little greedy with this and starts indenting things. I’m currently going through and adding semicolons everywhere it looks like they’re missing and then I’ll redo the format. I’ll try to get in early Monday to merge it, otherwise it will have to wait for some other end-of-day to conflict with just about every MR.
---
### Author:  [@seanm](https://discourse.vtk.org/u/seanm)
#### Post date: [November 15, 2019, 8:19pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/16 "2019-11-15T20:19:45Z")
Which macros?
You might be able to get the compiler to warn you if you reformulate the macros as do-while loops, then the compiler will error if there’s no semi-colon.
See also:
[https://wiki.sei.cmu.edu/confluence/display/c/PRE10-C.+Wrap+multistatement+macros+in+a+do-while+loop](https://wiki.sei.cmu.edu/confluence/display/c/PRE10-C.+Wrap+multistatement+macros+in+a+do-while+loop)
[https://wiki.sei.cmu.edu/confluence/display/c/PRE11-C.+Do+not+conclude+macro+definitions+with+a+semicolon](https://wiki.sei.cmu.edu/confluence/display/c/PRE11-C.+Do+not+conclude+macro+definitions+with+a+semicolon)
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 15, 2019, 8:28pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/17 "2019-11-15T20:28:25Z")
Some, yes. But other synthesize method implementations, so technically it will trigger `clang-tidy`'s “unnecessary” semicolon lint, but if `clang-format` is going to wreck the code without it, I’m going to call it “necessary”. I have 426 files edited right now for the semicolons, so I’m just going to wrap it up in the format change. I’ll have to do a buildbot run to get more modules than I can build locally looked at by a compiler though.
---
### Author:  [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)
#### Post date: [November 15, 2019, 9:56pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/18 "2019-11-15T21:56:10Z")
Or you could really start confusing people with more obscure semicolon swallowers 🙂
```auto
#define SOME_MACRO(x) \
if (x) \
{ \
foo(); \
} \
[](){}()
```
But I’m guessing that most of those are for `Set/Get` macros, and I can’t think of any good way to require a semicolon on them.
Edit: appending `struct {}` or `enum {}` work to silence `-Wextra-semi`, but throw new warnings under `-Wpedantic` ☹
---
### Author:  [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)
#### Post date: [November 16, 2019, 1:06pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/19 "2019-11-16T13:06:27Z")
> [@allison.vacanti](#):
>
> I can’t think of any good way to require a semicolon on them.
`clang-format` making the code look like a cascade of indentation is an effective, if unfortunate, way of “requiring” the semicolon.
---
### Author:  [@amaclean](https://discourse.vtk.org/u/amaclean)
#### Post date: [November 16, 2019, 11:16pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/20 "2019-11-16T23:16:33Z")
This is just a thought. You could bracket the macros in a:
```auto
// clang-format off
// Group of macro definitions ....
// clang-format on
```
Then there is no need to add semicolons. However the macros will need to be manually formatted in new code.
[Next page](https://discourse.vtk.org/t/clang-format-discussion/2073.md?page=2)