# 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:** 2

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [November 17, 2019, 11:07pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/21 "2019-11-17T23:07:27Z")

</div>

> [@amaclean](#):
>
> You could bracket the macros in a

It’s not the macros themselves; it’s the usage. And even that doesn’t hide the fact that there’s a missing semicolon; clang-format still indents afterwards (it just doesn’t _edit_ the bracked code code). Luckily, VTK is very good about putting `Macro` in macro names, so it was easy to find all the usages (though some may have escaped my search, their telltale diff pattern also doesn’t show up in a scan of the patch).

## Status

The rest may be TL;DR for some, so if you just want the diff, I’ve updated the `clang-format` branch on my fork. I have the buildbots testing it overnight (kwrobot needs to be told to check formatting before it can be merged and it will likely need a rebase for any other changes anyways). To those interested in some things that were done to make formatting work for VTK and notes for improvements I noticed, feel free to continue.

## Manual hinting

I’ve perused the diff and gotten it down to a ~1.7M line patch that removes a net 50k lines (the initial, naïve run added a net 400k lines with a 2.5M line patch) by fixing some things. Things I found:

- imported or generated code (fixed by excluding them from formatting)
- data arrays; I added hints to clang-format to format them the way they probably should be formatted and/or moved them to separate, ignored files.
- hiding manually formatted code from the formatter (usually horizontally aligned line ups for mathematical formulas; usually after applying sensible diffs `clang-format` wanted to make)

## “Bad” patterns seen while looking over the diffs

This isn’t _too_ common, but something to watch out for.

- Lots of tests which do SOA-style storage for test data rather than AOS.
- Some data storage does `int foo[][8] = {}` and then has comments for what each element is for. Instead, an anonymous `struct` should be used to give each value set a name. Filters/\* and Common/DataModel are the main culprits here.
- Lots of point storage all over the place doing `arr[npts * 3]` rather than `arr[npts][3]`. Cell types and such do this a lot.
- Lots of arrays that aren’t marked `static` and/or `const`. Most should probably become `constexpr` anyways. Happens ~everywhere in Filters/\* and Common/DataModel.
- Lots of the mathematical formula code doesn’t factor out common subexpressions like `(1 + z) * (1 - z)` and instead recalculate it a dozen times (just search for “derivative” case insensitively and you’ll find most of them). These sometimes, but not always, needed manual formatting hints if you want to look at that commit for a place to start.
- There are many places where assignments were formatted in a matrix-style block. `clang-format` has split these into an assignment-per-line; proper matrix data structures should probably just be preferred rather than treating `double[9]` as a 3x3 matrix.

I saw lots of code that does:

```c++
expr1; expr2; // comment

```

which became:

```c++
expr1;
expr2; // comment

```

when it should probably be:

```c++
// comment
expr1;
expr2;

```

These are too hard to find mechanically, so I let `clang-format` do its thing.

## Getting good formatting for data arrays

Things I learned about array formatting doing this:

```c++
int arr[] = { 0, 1 };

```

is well-formatted, but this is the proper format with a trailing comma:

```c++
int arr[] {
  0,
  1,
};

```

So if you want your arrays to be one element per line, add a trailing comma, otherwise don’t. This is how I hinted to `clang-format` how to format various data arrays (the hinting is a separate commit for anyone curious).

---

<div class="post-metadata">

**Author:** ![amaclean](https://discourse.vtk.org/user_avatar/discourse.vtk.org/amaclean/32/224_2.png) [@amaclean](https://discourse.vtk.org/u/amaclean)\
**Post date:** [November 18, 2019, 4:34am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/22 "2019-11-18T04:34:56Z")

</div>

@ben.boeckel , thanks for clarifying this, obviously my view was quite simplistic, I didn’t realise that it was the usage. I like your comments on making formatting work and notes for improvements.

---

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [November 18, 2019, 9:34pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/23 "2019-11-18T21:34:50Z")

</div>

The MR has now been merged.

---

<div class="post-metadata">

**Author:** ![dgobbi](https://discourse.vtk.org/user_avatar/discourse.vtk.org/dgobbi/32/18_2.png) [@dgobbi](https://discourse.vtk.org/u/dgobbi)\
**Post date:** [November 18, 2019, 11:33pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/24 "2019-11-18T23:33:55Z")

</div>

Now the doxygen comments in the header files wrap at around 70 or 80 columns (as per our old standard), but the code itself wraps at 100 columns. It’s readable but it looks kind of funny.

---

<div class="post-metadata">

**Author:** ![allison.vacanti](https://discourse.vtk.org/user_avatar/discourse.vtk.org/allison.vacanti/32/679_2.png) [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)\
**Post date:** [November 18, 2019, 11:39pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/25 "2019-11-18T23:39:55Z")

</div>

It might be too late, but would it be possible to change the current pointer/ref syntax from `double* ptr;` to `double *ptr`?

A _lot_ of code in VTK still follows the C-ism of listing variables at the top of the function, often with multiple declarations per line. This can lead to misleading-looking code such as

```auto
vtkIdType* cells, npts, ptId;

```

which, at a glace, looks like all three variables are pointers, while

```auto
vtkIdType *cells, npts, ptId;

```

is (a bit) clearer. It also looks like discourse’s markdown syntax highlighter handles it better 🙂

Ideally, we’d only declare one var per line and only declare them when they’re needed to keep scopes manageable, but there’s a lot of legacy code that doesn’t follow those practices.

---

<div class="post-metadata">

**Author:** ![amaclean](https://discourse.vtk.org/user_avatar/discourse.vtk.org/amaclean/32/224_2.png) [@amaclean](https://discourse.vtk.org/u/amaclean)\
**Post date:** [November 19, 2019, 4:32am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/26 "2019-11-19T04:32:25Z")

</div>

@ben.boeckel I did a build in Windows and it failed at:

```nohighlight
...\VTK\Common\Core\vtkWin32OutputWindow.cxx
...VTK\Common\Core\vtkWin32OutputWindow.cxx(351): error C2143: syntax error: missing ';' before 'this'
...\VTK\Common\Core\vtkWin32OutputWindow.cxx(358): error C2143: syntax error: missing ';' before 'this'

```

I turns out that for some reason I had `VTK_LEGACY_REMOVE:BOOL=OFF` in the `CMakeCache.txt` file. Once I set it to `ON` everything built OK.

---

<div class="post-metadata">

**Author:** ![amaclean](https://discourse.vtk.org/user_avatar/discourse.vtk.org/amaclean/32/224_2.png) [@amaclean](https://discourse.vtk.org/u/amaclean)\
**Post date:** [November 19, 2019, 5:28am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/27 "2019-11-19T05:28:16Z")

</div>

Good point @allison.vacanti, it would be a bit clearer.

#### A bit of history

If you want to see some really old code look at `./Common/DataModel/Testing/Cxx/otherFieldData.cxx` .  
Way back in 2003 Ken Martin had to modify BerkGeveci’s original test function: `int otherFieldData(int, char*[])`):

```auto
  for(int i=0; i<5; i++)
    {
    // ...
    }

```

to:

```auto
  int i;
  // ...
  for(i=0; i<5; i++)
    {
    // ...
    }

```

This was a Visual Studio 6 requirement! This code has remained the same ever since. You’ll see this pattern in a lot of the older code.

gcc 2.7, way back in 1995, was the last gcc compiler to require this. Unfortunately VS6 remained in use for a long time so a lot of coders used this pattern. VS6.0 was introduced in 1998 however I can assure you that one of my ex-colleagues was still coding in it in 2012 when I retired. I could not get him to update!!!

Old habits die hard, for instance it pops up in `Common/Core/vtkMersenneTwister_Private.cxx`.

The pattern still pops up, see: [[What is “ Variable ’ i ’ was not declared in scope ” in c++?](https://stackoverflow.com/questions/54146989/what-is-variable-i-was-not-declared-in-scope-in-c)] note that one of the respondents is still using VS6. People got around this issue by declaring `i` outside the first loop.

---

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [November 19, 2019, 8:37am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/28 "2019-11-19T08:37:54Z")

</div>

-1 for @allison.vacanti suggestion. Let’s not enforce these C-ism, it should instead be converted to :

```
 vtkIdType* cells;
 vtkIdType npts, ptId;
```

---

<div class="post-metadata">

**Author:** ![allison.vacanti](https://discourse.vtk.org/user_avatar/discourse.vtk.org/allison.vacanti/32/679_2.png) [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)\
**Post date:** [November 19, 2019, 12:34pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/29 "2019-11-19T12:34:29Z")

</div>

Thanks for the background, @amaclean. It’s an especially unfortunate issue for maintainers: trying to read a lengthy function and trace back a variable hundreds of lines when it could have just been scoped to 10-15 lines…things get complicated quickly!

@mwestphal That snippet still doesn’t follow most C++ style guidelines, which would put every variable on its own line. I’m certainly not in favor of enforcing these c-ism, but since our code is already filled with them, I would prefer a syntax that doesn’t complicate reading them. This issue is a common stumbling point, especially for beginners.

Even without these multivar decls, the `&`/`*` modifiers are associated with the variable’s identifier, not the type, and it makes more sense to group them this way IMO.

---

<div class="post-metadata">

**Author:** ![mwestphal](https://discourse.vtk.org/user_avatar/discourse.vtk.org/mwestphal/32/19_2.png) [@mwestphal](https://discourse.vtk.org/u/mwestphal)\
**Post date:** [November 19, 2019, 1:08pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/30 "2019-11-19T13:08:24Z")

</div>

> <https://stackoverflow.com/questions/2660633/declaring-pointers-asterisk-on-the-left-or-right-of-the-space-between-the-type>

Indeed a matter of personal preference.

---

<div class="post-metadata">

**Author:** ![allison.vacanti](https://discourse.vtk.org/user_avatar/discourse.vtk.org/allison.vacanti/32/679_2.png) [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)\
**Post date:** [November 19, 2019, 1:29pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/31 "2019-11-19T13:29:27Z")

</div>

It’s preference as far as they both compile, though `type *id;` represents how the compiler parses the expression and is more readable in certain situations (which VTK is chock full of).

Meanwhile, `type* id;` …still compiles 🙂 I’ve honestly never heard a good reason to write it this way other than “it works too.”

---

<div class="post-metadata">

**Author:** ![estan](https://discourse.vtk.org/user_avatar/discourse.vtk.org/estan/32/460_2.png) [@estan](https://discourse.vtk.org/u/estan)\
**Post date:** [November 19, 2019, 6:45pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/32 "2019-11-19T18:45:31Z")

</div>

Over in Qt, this age old question was quite recently brought up on the Qt developer mailing list, if you want some bike shedding inspiration from another camp 🙂 [https://lists.qt-project.org/pipermail/development/2019-October/037701.html](https://lists.qt-project.org/pipermail/development/2019-October/037701.html)

That thread was started with someone wanting to go the opposite way though - make sure the star is always with the type - as they felt it was the more in line with what C++ outside of Qt does. Anyway, as expected, the thread ended with no action.

---

<div class="post-metadata">

**Author:** ![amaclean](https://discourse.vtk.org/user_avatar/discourse.vtk.org/amaclean/32/224_2.png) [@amaclean](https://discourse.vtk.org/u/amaclean)\
**Post date:** [November 20, 2019, 9:06am UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/33 "2019-11-20T09:06:16Z")

</div>

@allison.vacanti agree with you, I use ˋtype \*id;ˋ it is much clearer IMHO. However it is not worth the effort of changing all the VTK code. @mwestphal your idea would make things much clearer.

---

<div class="post-metadata">

**Author:** ![allison.vacanti](https://discourse.vtk.org/user_avatar/discourse.vtk.org/allison.vacanti/32/679_2.png) [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)\
**Post date:** [November 20, 2019, 12:59pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/34 "2019-11-20T12:59:12Z")

</div>

> However it is not worth the effort of changing all the VTK code

Normally I’d agree and I wouldn’t bring something like this up, but we already changed all of the code to be `type* id;` two days ago . We could change a parameter in a config file, rerun a script, and commit the result and most developers wouldn’t notice that there were two major style changes instead of just the one 🙂

But I’ll stop advocating for this now. If we want to change it, great! If not, meh. Using this opportunity to fix the syntax with minimal additional disruption makes sense to me, but it’s ultimately not a major issue.

---

<div class="post-metadata">

**Author:** ![danlipsa](https://discourse.vtk.org/user_avatar/discourse.vtk.org/danlipsa/32/390_2.png) [@danlipsa](https://discourse.vtk.org/u/danlipsa)\
**Post date:** [November 20, 2019, 2:46pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/35 "2019-11-20T14:46:44Z")

</div>

Note Google style guide allows attaching the modifier to either the variable or the type,  
but does not allow declaring pointers and values in the same declaration.

[https://google.github.io/styleguide/cppguide.html#Pointer\_and\_Reference\_Expressions](https://google.github.io/styleguide/cppguide.html#Pointer_and_Reference_Expressions)

---

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [November 20, 2019, 3:05pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/36 "2019-11-20T15:05:28Z")

</div>

> [@amaclean](#):
>
> I turns out that for some reason I had `VTK_LEGACY_REMOVE:BOOL=OFF` in the `CMakeCache.txt` file. Once I set it to `ON` everything built OK.

Well, that’s a place I missed a semicolon. I missed the `VTK_LEGACY_REPLACED_BODY` calls that were split over 2 lines and didn’t see the error since buildbots build with LEGACY\_REMOVE=ON.

> [@amaclean](#):
>
> Old habits die hard, for instance it pops up in `Common/Core/vtkMersenneTwister_Private.cxx` .

That file just needs to die. [https://gitlab.kitware.com/vtk/vtk/issues/17730](https://gitlab.kitware.com/vtk/vtk/issues/17730)

As for the pointer thing. The “attach to the variable” is a stupid C-ism. The pointer belongs to the type that is declared.

> [@danlipsa](#):
>
> does not allow declaring pointers and values in the same declaration.

Is this something we can get `clang-tidy` to tell us about?

---

<div class="post-metadata">

**Author:** ![allison.vacanti](https://discourse.vtk.org/user_avatar/discourse.vtk.org/allison.vacanti/32/679_2.png) [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)\
**Post date:** [November 20, 2019, 3:43pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/37 "2019-11-20T15:43:11Z")

</div>

> [@ben.boeckel](#):
>
> The pointer _should_ belong to the type that is declared.

FTFY 🙂 It should be that way, but it isn’t.

---

<div class="post-metadata">

**Author:** ![ben.boeckel](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/b/ea5d25/32.png) [@ben.boeckel](https://discourse.vtk.org/u/ben.boeckel)\
**Post date:** [November 20, 2019, 4:41pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/38 "2019-11-20T16:41:55Z")

</div>

However, my preferred solution is to avoid/prevent the confusing case, not to bow down to it 🙂 .

---

<div class="post-metadata">

**Author:** ![allison.vacanti](https://discourse.vtk.org/user_avatar/discourse.vtk.org/allison.vacanti/32/679_2.png) [@allison.vacanti](https://discourse.vtk.org/u/allison.vacanti)\
**Post date:** [November 20, 2019, 4:51pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/39 "2019-11-20T16:51:30Z")

</div>

My preferred solution is to make my mental model match what’s actually happening. Which means:

> [@ben.boeckel](#):
>
> bow down to it

is 100% correct – I admittedly have Stockholm Syndrome with C++ 😛

Embrace the ugliness!

---

<div class="post-metadata">

**Author:** ![seanm](https://discourse.vtk.org/letter_avatar_proxy/v4/letter/s/a88e4f/32.png) [@seanm](https://discourse.vtk.org/u/seanm)\
**Post date:** [November 20, 2019, 5:13pm UTC](https://discourse.vtk.org/t/clang-format-discussion/2073/40 "2019-11-20T17:13:33Z")

</div>

Well, my 2¢: I greatly prefer `type* var`.

I’m happy to see this formatting work, but personally I think it got merged way too fast. I first heard of it at the recent hackathon and it was merged just 7 business days later. For people like me that don’t work on VTK every day, that left no time to review it before.

Anyway… Now that this mega change has already occurred, I’d vote against another mega change of asterisk position.

There is this clang-tidy check:  
[https://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-declaration.html](https://clang.llvm.org/extra/clang-tidy/checks/readability-isolate-declaration.html)

It seems to have done at least one weird thing to Objective-C++ code like:

[https://gitlab.kitware.com/vtk/vtk/blob/master/Examples/GUI/Cocoa/MyWindowController.mm#L64](https://gitlab.kitware.com/vtk/vtk/blob/master/Examples/GUI/Cocoa/MyWindowController.mm#L64)

where `->` got spaces added before and after. I couldn’t find any clang-format option related to this, but don’t know the tool well.

BTW is it normal that gitlab is not colouring Obj-C++ code at all?

Also, you might all find this interesting:  
[https://travisdowns.github.io/blog/2019/11/19/toupper.html](https://travisdowns.github.io/blog/2019/11/19/toupper.html)

Sean

[Previous page](https://discourse.vtk.org/t/clang-format-discussion/2073.md?page=1)

[Next page](https://discourse.vtk.org/t/clang-format-discussion/2073.md?page=3)