use override attribute for virtual functions

[KrisThielemans]

C++-11 allows using the override attribute to prevent errors where inadvertently the wrong signature is used, see e.g. https://en.wikipedia.org/wiki/C%2B%2B11#Explicit_overrides_and_final

#826 introduces this for the BinNormalisation hierarchy, but there are far more of course. It's unfortunately not obvious how to automate this.

[mahnen]

After coniguring the build in a

build/

folder, with -DCMAKE_EXPORT_COMPILE_COMMANDS=ON

so that

build/compile_commands.json

exists. Then with clang-tidy like this:

run-clang-tidy -p build/ -header-filter=.* -checks='-*,modernize-use-override' -fix

see:

https://www.kdab.com/clang-tidy-part-1-modernize-source-code-using-c11c14/

[mahnen]

I tried this in a custom branch, see pull request number 23 (closed), it kind of worked well. The above command takes quite a while to run, unfortunately, due to the large codebase.

Maybe you can select a few of the clang-tidy methods that you like and I can run them? Maybe also :

modernize-use-nullptr¶

It seems a few of them will break compatibinitly with the older compilers in the CD

[KrisThielemans]

thanks @mahnen I think this is a great idea. I suppose the problem with #867 was:

• possible "too modern". I suppose clang-tidy should have settings for C++ version (STIR is still on C++-11).
• automated tools can struggle with our work-arounds. For instance, the appveyor jobs failed because the automatic tool couldn't cope with a manual work-around for (very old versions of Visual Studio). Of course, those work-arounds are actually out-of-date already, so probably should be removed first.
• changing 460 files means a lot of code-review. (If it is only a matter of running an automatic tool, that'd be easier than if corrections than needed to be made by hand, but still some checking would be good.)

I think doing this in stages would solve these problems though. It should be paired with creating some scripts for doing the cleaning, such that people can run it on a PR. We could even ultimately do that via git hooks once #724 is merged (although run-time might become a problem).

Do you have a feel for a good order of doing this?

[mahnen]

I'll try with the minimum modernize-use-override and create a pull req. Lets see ;)

[KrisThielemans]

sure. If possible, do the PR in commits: first commit to add a script (in scripts/maintenance), another that has the result.

Presumably running the script twice doesn't change anything anymore, so we can always run this after future merges.

[KrisThielemans]

If we can find a volunteer to go and remove all work-arounds with STIR_NO_NAMESPACES and a lot of the _MSC_VER ones, that'd also help 😉

[mahnen]

pull request created, for your review #868

[KrisThielemans]

I'm picking this up now, as we've removed all (?) the work-arounds and macros for old compilers.

I'm struggling with this sadly. Here's a list of steps that I tried, not in logical order...

1. trying with clang-tidy-14 installed with APT on Ubuntu 22.04, using an existing build that uses gcc
   • I had complaints about omp.h not found. sudo apt install libopenmp-dev didn't help. Any file with omp.h therefore didn't get processed.
   • I had warnings that it cannot modify /usr/include/hdf5/serial/H5*h, even though these should be "system files". I thought this might be because in a few places we include "H5Cpp.h" as opposed to <H5Cpp.h> but changing that didn't help. However, these warnings don't prevent the tool running.
2. installing clang-tidy-17, llvm-openmp and CMake 3.28 from conda-forge in a conda env
   • still had the omp.h problem, despite installing llvm-openmp (and omp.h being present).
3. I figured CMake had to be run using clang, as otherwise the generated json contains gcc locations, which don't necessarily work for clang. However, the conda-env CMake couldn't find openmp_c libraries (needed for parallelproj).
4. Still in the conda-env, I disabled openmp and parallelproj when running CMake. That worked ok.
5. running clang-tidy (17) on that build complained about boost/config.hpp not found, but that is a system file (it is in /usr/include). (same comment about use of "" vs <> but it didn't help either). As stir/config.h includes a boost file, this meant clang-tidy didn't change any header at all.
6. I went back to running clang-tidy-14 on the CMake config generated for clang-17. Aside from the warnings on HDF5 files, this ran ok.

Surely this isn't how it should be. I'm assuming this is because I'm making some mistakes in my installation, but I don't know which ones. I think a working summary could be

1. disable openmp and parallelproj in a CMake build (probably doesn't matter which, but include as many dependencies as you can, as otherwise they won't be processed)
2. Use CMake to export the compilation database as above
3. run run-clang-tidy as above
4. run remaining files by hand

See #1367

[KrisThielemans]

Closed by #1367