Code: Use clang-format 13

[hoffie]

Short description of changes

• Coding style: Require clang-format 13
  We currently use clang-format 10 which has shown to produce unexpected output. This is annoying for PR submitters as the CI forces non-optimal code layout, especially when submitters use a later clang-format version which does not show this behavior.
  This commit changes CI to use clang-format 13 and updates relevant clang-format inline docs.

• Code: Run clang-format 13 on all source files

CHANGELOG: Code: Updated clang-format version requirement and CI check to v13 due to bugs in v10.

Context: Fixes an issue?

Related: #2552

Does this change need documentation? What needs to be documented and how?

Yes. This necessary documentation update of CONTRIBUTING.md has part of this PR.

Status of this Pull Request
Ready.

What is missing until this pull request can be merged?

Checklist

• I've verified that this Pull Request follows the general code principles
• I tested my code and it does what I want: CI used v13
• My code follows the style guide: Heh...
• I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
• I've filled all the content above

[softins]

This has the potential to be a pain. Do we know what version of clang-format is included with various distributions of Linux and macOS? Ubuntu 20 has clang-format version 10. Raspbian 11 (the latest) has clang-format version 11.

When I was developing on Raspbian 10, which only has clang-format 7, it was annoying to have to push on the Pi, fire up an Ubuntu 20 VM, pull to it, do clang-format and push back the changes. It became much nicer when I switched to Raspbian 11, as I could do clang-format natively. Moving to a requirement of clang-format 13 would be a backward step for me.

I suppose I might be able to install a newer clang-format from source, but I prefer to use what is packaged with the distro.

[softins]

NOTE: I had not read the issue #2552 when I typed the above. I'm still not keen.

[hoffie]

I'm still not keen.

I understand that regarding the consequences (worsened situation for many Linux users) and I agree that we should try to avoid that. I think this was the main reason for choosing a rather conservative clang-format version when first introduced. However, if we don't update, the only other alternatives I can think of are adding workarounds for wrongly-formatted code or merging PRs despite red warnings (we might be able to reduce the checks to the files which are modified in a PR, though). Do you have any other ideas?

[softins]

I haven't encountered any formatting issues yet, and have not studied the examples mentioned in the issue. Has it been determined whether the observed issues are fixed in 11, 12 or 13?

[softins]

Maybe if clang-format fails in CI it could raise a PR on the contributor's repo for the required changes? I don't know how hard that would be, or even if it's possible.

[hoffie]

I haven't encountered any formatting issues yet, and have not studied the examples mentioned in the issue. Has it been determined whether the observed issues are fixed in 11, 12 or 13?

The one we've pinpointed exactly (pointer receivers) is fixed in 13. This has already been relevant in a PR by @pgScorpio so far and not on master.
Another one is relevant on master and can be seen as part of this PR: 18e45d0#diff-c044ba02ccf7862f1e2f56436d5b92baa7bb35601791846655fb07ac253a8697

Maybe if clang-format fails in CI it could raise a PR on the contributor's repo for the required changes? I don't know how hard that would be, or even if it's possible.

I think that's doable, but requires contributors to merge the PR and pull from their own PR (or do it again after the next force push).

[softins]

Another one is relevant on master and can be seen as part of this PR: 18e45d0#diff-c044ba02ccf7862f1e2f56436d5b92baa7bb35601791846655fb07ac253a8697

That one looks like clang-format getting confused because of rapid turning off and on of clang-format. I couldn't even tell from scrolling back a bit what the indentation level should be! Is it really necessary to have all possible version compatibility sections un-indented? Might it be better to delimit them with conspicuous special comments instead and allow them to be subject to clang-format indentation?

[hoffie]

I'm not a fan of the un-ident at all, but that's how it is. If there's agreement to change that (@pljones? :)), that would work around this part.
Sadly, it's just one of the issues. Another one is this and I can't think of a workaround (despite accepting it or disabling clang-format for it): #2407 (comment)

[pljones]

I'm not a fan of the un-ident at all, but that's how it is. If there's agreement to change that (@pljones? :)), that would work around this part.

Hey, I just live here... Uh... In fact, the outdented style for "Look, I'm not normal code, so pay attention" is a bit odd. Having a #ifdef rather than commenting it out (here) would be better - I could have complained in Jamulus.pro that you couldn't do both serveronly and testbench, for example. I was left having to add a #ifdef around a comment, in case someone was working with testbench enabled...

[pgScorpio]

@hoffie

Do you have any other ideas?

Just some thoughts.... (As we always will have to deal with different clang-format-versions I guess.)

• We could make the auto-build check use multiple versions of clang-format, if one of them gives an OK result, the check should be considered OK.
• Can we include the clang-format binaries in the repo (in the OS folders) and let make clang_format use those versions. (This would limit the amount of different versions to be checked by auto-build.)
• Letting make clang_format adding a comment in the file with the clang-format version (output of clang-format --version) could also help to determine which clang-format version to use for the check.

[hoffie]

Thanks for those suggestions.

• We could make the auto-build check use multiple versions of clang-format, if one of them gives an OK result, the check should be considered OK.

This would work for new code, but would be annoying for existing code which might flip back and forth dependening on the PR submitter. This increases diff sizes and makes git blame less useful.

• Can we include the clang-format binaries in the repo (in the OS folders) and let make clang_format use those versions. (This would limit the amount of different versions to be checked by auto-build.)

Hm, if we go that route I'd rather point to some canonical source to get the binaries. Git repos usually aren't supposed to contain binaries. We'd need to have at least four versions: Linux x86_64, Linux armhf, Windowx 86_64, Mac x86_64. In addition, make clang_format is just one way to format code. Several people rather use the functionality included within their IDEs or editors.

• Letting make clang_format adding a comment in the file with the clang-format version (output of clang-format --version) could also help to determine which clang-format version to use for the check.

Hmm, but I guess this would have the same back-and-forth problem for all non-new files as well? Or would require submitters to have multiple clang-format versions installed in parallel?

[hoffie]

What problems would we expect if we switched to v13 as the authoritive version, but advised users only to clang-format the files which they've touched? I think that's what most editors do anyway. We could then drop make clang_format again (which does touch all files) and instead advise using git-clang-format (which I only found right now).

That way, the only known formatting conflict in current master would be the one in src/settings.cpp.

[ann0see]

Related: #2600

[ann0see]

Closing as I think #2600 is a short term fix. As soon as debian stable moves to a more recent Clang format version, we can re-open this.