formatting depends on clang-format version (and the one I used created ugly things)

KrisThielemans commented 7 months ago

I ran clang-format on Ubuntu 22.04. This turns out to be version 14.0.0-1ubuntu1.1. This is also the version run by the pre-commit-check action, https://github.com/UCL/STIR/blob/ee307c721cae562ab163aee06dbab8407d861414/.github/workflows/pre-commit-check.yml#L12 so it's happy on master.

Problem

However, I now ran it with latest clang-format from conda-forge, which is version 17.0.6. This gives changes in probably 40 files. This is probably a well-known problem, see e.g. https://stackoverflow.com/questions/59395507/how-to-maintain-a-clang-format-file-for-different-clang-format-versions and the post it refers to. It degrades my opinion on clang-format dramatically.

Probably the only solution to this is to enforce a clang-format version, which can be done via https://pre-commit.ci. I rather dislike this solution: people set-up their editor carefully, they commit some code and push, pre-commit.ci reformats it. They have to pull (presumably). Now their editor reformats it back again. etc. etc. (I guess this would lead the our number of commits increasing with a factor 2, and git fame reporting wrong results).

Another solution is to use a more stable tool than clang-format, and therefore likely change 1000+ files again.

@casperdcl @dvolgyes @markus-jehl any suggestions?

More detail

I'll create a PR with the 17.0.0 version such that the changes can be expected, but I've picked 3 illustrating 3 different categories:

trivial, presumably could be configured to stick to the 14 version.
weird: the 14 version made more sense. This category is worrying as presumably this is due to bugs in 17.0.0, which will be
beneficial: clean-up a total mess introduced by 14.0.0

trivial

#include #include -//#include +// #include ```

weird

``` diff --git a/src/recon_buildblock/distributable.cxx b/src/recon_buildblock/distributable.cxx index d422a0723..00db9c610 100644 --- a/src/recon_buildblock/distributable.cxx +++ b/src/recon_buildblock/distributable.cxx @@ -627,12 +627,12 @@ LM_distributable_computation(const shared_ptr PM_sptr, std::vector measured_div_fwd; #ifdef STIR_OPENMP # pragma omp parallel shared(local_output_image_sptrs, \ - local_row, \ - local_log_likelihoods, \ - local_counts, \ - local_count2s, \ - local_measured_bin, \ - local_fwd_bin) + local_row, \ + local_log_likelihoods, \ + local_counts, \ + local_count2s, \ + local_measured_bin, \ + local_fwd_bin) #endif ```

beneficial

the following code snippet and the lines below it https://github.com/UCL/STIR/blob/ee307c721cae562ab163aee06dbab8407d861414/src/test/numerics/test_matrices.cxx#L160-L166

KrisThielemans commented 7 months ago

some other posts

https://egli.dev/posts/update-alternatives-for-clang-format/ says how to select a version in ubuntu
https://youtrack.jetbrains.com/issue/CPP-15236/Support-custom-clang-format-binary says that CLion bundles its own version, so it cannot be forced to a fixed version.

KrisThielemans commented 7 months ago

For now, all PRs should be run with clang-format 14.0.0.

robbietuk commented 6 months ago

https://youtrack.jetbrains.com/issue/CPP-15236/Support-custom-clang-format-binary says that CLion bundles its own version, so it cannot be forced to a fixed version.

This might not be true anymore, and from CLI

$ which clang-tidy
/usr/bin/clang-tidy
$ clang-tidy --version
Ubuntu LLVM version 14.0.0

  Optimized build.
  Default target: x86_64-pc-linux-gnu
  Host CPU: alderlake

KrisThielemans commented 6 months ago

that could help, except of course we're after clang-format (or does the former include the latter?)

UCL / STIR