STIR icon indicating copy to clipboard operation
STIR copied to clipboard

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

Open KrisThielemans opened this issue 1 year ago • 4 comments

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

diff --git a/src/utilities/construct_randoms_from_singles.cxx b/src/utilities/construct_randoms_from_singles.cxx index 86c234429..d76ae5ddb 100644 --- a/src/utilities/construct_randoms_from_singles.cxx +++ b/src/utilities/construct_randoms_from_singles.cxx @@ -27,7 +27,7 @@ #include <iostream> #include <fstream> #include <string> -//#include <algorithm> +// #include <algorithm>

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<ProjMatrixByBin> PM_sptr,
   std::vector<float> 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 avatar Feb 09 '24 00:02 KrisThielemans

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 avatar Feb 09 '24 00:02 KrisThielemans

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

KrisThielemans avatar Feb 09 '24 00:02 KrisThielemans

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, image 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

robbietuk avatar Feb 26 '24 18:02 robbietuk

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

KrisThielemans avatar Feb 26 '24 22:02 KrisThielemans