Infrastructure: Update bundled clang-format to version 16

[tamiko]

This pull request updates the bundled clang-format version to 16.0.6.

This new version gets rid of an annoying long-standing pet-peeve (see changes)
and has better C++17/C++20 compatbility (admittedly, for issues we seem to have
avoided). Nevertheless I think it is time to upgrade.

TODO:

• build clang-format 16 on oldest infrastructure available (glibc 2.17).
• add clang-format 16 binary to latest release and update download-clang-format script.

Changes: https://github.com/tamiko/dealii/pull/16/files

[kronbichler]

I like the improvements, even though this is going to be an annoying PR because it will make essentially all open PRs conflict. Still, I am in support.

[masterleinad]

We should also use

QualifierAlignment: Left

[bangerth]

That's painful, but it probably has to happen at some point.

github has real trouble showing all of the changes. Most seem to be related to the alignment of & or *. Is the rest basically the same?

[bangerth]

If we want to increase the pain level even further, could I advocate for a line length of 100 or 120 characters again?

We may want to do this in two steps. It is conceivable that the current patch doesn't actually break all that many pending patches, whereas increasing the line length definitely will.

[tamiko]

@bangerth Yes, mainly the alignment of & and * changed.

I have removed the indent application from this PR to show all changes that I have done so far. I will link to a PR against my own branch to show all changes in the source code indenting.

[tamiko]

Changes are here: https://github.com/tamiko/dealii/pull/16/files

[tamiko]

@bangerth Increasing the line limit will be a really tough sell for me.

You can see the result here: https://github.com/tamiko/dealii/tree/update_clang_format-3
This is a +150K/-300k lines change.

For starters, I don't buy the argument that it increases readability: To the contrary, I have real problems parsing a line of source code that is more than 100 characters long (mainly due to the increased eye movement necessary to parse all of it).
What doesn't help here is the fact that clang-format will happily max out the available 100/120 character limit instead of using it sparingly.

But more importantly, I think that increasing the line limit is more akin to treating the symptom than the cause. The main issue we have is that we tend to have too many levels of nested control flow and often try to cram too much into a single statement. The solution to this should be to refactor such control flow logic and to use a much more declarative style for statements (and not to increase the line limit which encourages this even more).

But if a majority really wants to relax the 80 character rule, I have an alternative suggestion: What about we simply dispense of the character limit altogether. Setting the column width to 0 will inform clang-format to respect manual line breaks as much as possible while enforcing everything else. This would also mean that we don't have to reflow anything to begin with. We might also want to augment this with adding a maximal line width check to our indent script - otherwise we end up with monstrously long lines. (I would still viscerally hate this... but it's not a "from my cold, dead hands" situation any more.)

[bangerth]

On 8/21/23 18:32, Matthias Maier wrote: But if a majority really wants to relax the 80 character rule, I have an alternative suggestion: What about we simply dispense of the character limit altogether. Setting the column width to |0| will inform |clang-format| to respect manual line breaks as much as possible while enforcing everything else. This would also mean that we don't have to reflow anything to begin with. We might also want to augment this with adding a maximal line width check to our indent script - otherwise we end up with monstrously long lines. (I would still viscerally hate this... but it's not a "from my cold, dead hands" situation any more.)

:-) I see you have truly arrived in this country, quoting Charlton Heston. Line length limit of zero: This actually sounds quite good to me (as a second, separate step). Why did we not do this in the original conversion? Rewriting 15 years of our choices of line endings has of course always been my main complaint :-(

[bangerth]

Many of the non-whitespace changes are ones where it converts X const into const X. Let me deal with this separately -- I bet I can get 90% of those by script. The more we can shrink this patch, the better.

[tamiko]

@bangerth

Why did we not do this in the original conversion?

I don't know anymore. Honestly, let's not revisit that discussion again.

[tamiko]

Agreed. Let's minimize the impact. I'll make this a conversion to clang-format-16 only. No further changes (except the switch to c++20).

[bangerth]

On 8/21/23 20:57, Matthias Maier wrote: I don't know anymore. Honestly, let's not revisit /that/ discussion again.

Ha, no, let's not do that :-)

[kronbichler]

@bangerth Increasing the line limit will be a really tough sell for me.

Personally I do have some sympathies for a higher line limit, but definitely not more than 100 characters for the reasons listed by @tamiko. In fact, 100 lines would be a real win in certain places in my opinions. Yes, we should try to not nest as much control flow as we do now, but I often see myself in the situation that refactoring would take much more time. Furthermore, 80 character lines are, to some extent, in conflict with descriptive variable names and our indentation habits (2 characters before opening braces {, 2 more inside, giving up 4 characters per sub-block). Inside internal namespaces and some classes, we often lose 16 to 20 characters here. In that sense, 100 characters per line would bring us back the 80 character limit. 😉

But if a majority really wants to relax the 80 character rule, I have an alternative suggestion: What about we simply dispense of the character limit altogether. Setting the column width to 0 will inform clang-format to respect manual line breaks as much as possible while enforcing everything else. This would also mean that we don't have to reflow anything to begin with. We might also want to augment this with adding a maximal line width check to our indent script - otherwise we end up with monstrously long lines.

I like that clang-format breaks lines for me, it often does it more consistently than what I would do. And from reviewing code before line limits were enforced I think that this automatic step to break overly long lines was one of the major advances. I would certainly not be opposed to let the line breaks happen at the writer's discretion, but how are we going to make this work in practice? Again judging from my personal point of view, I would like a tool to break lines for me, say at the strict upper limit of 100 characters, to be conforming with whatever the indent CI will later check. But then I would like to be able to break earlier or in different places if I as a human judge that clang-format (or any other tool X) does not break where I want the break to be. Not sure if this "dream" on the user having the last word over clang-format is achievable, but the fundamental aspect to enforce locally what the indent CI does is important. Also, many less experienced developers should have a simple tool at hand to conform with this particular aspect of line breaks (or any other limit that we let not enforce by an automatic tool, but have the CI complain, if it is a frequent "mistake").

[bangerth]

I can report that if I download this patch, extract all non-whitespace changes, apply them locally, and then run ./contrib/utilities/indent on them, I get back a clean version. In other words, there is nothing left to take from this patch (like what we did in #15909 and #15908) that would make our life easier.

I will note that the changes here are "only" 20,000 lines, out of a total of 1.5M lines in deal.II. It's quite possible that the pain will be limited on pending patches.

[bangerth]

You'll have to resolve merge conflicts.

[bangerth]

Let's break out the popcorn to observe the mayhem!