Add clang-tidy to implement some C++17 language features
Issue/feature addressed
Transitioning to a more modern C++ standard is important to improve interoperability with other libraries and application development.
Proposed solution
This approach was originally implemented within !1427 (closed); however too many changes were introduced there leading to the MR becoming unreasonably large. In this MR we aim to implement some smaller features of C++17, based on the automatic use of clang-tidy
: a more feature-complete linter for C++ code than simple formatting tools such as clang-format
.
Implementation
clang-tidy
has been configured using the root-level .clang-tidy
file to enable the following checks/adjustments:
-
readability-braces-around-statements
: enforces the use of braces around statements. This corrects a number of files in which we have used e.g. single lineif
orfor
statements. -
modernize-concat-nested-namespaces
: namespace concatenation, i.e. use ofnamespace Nektar::LibUtilities
. This is the only C++17 feature enabled in this branch. -
modernize-raw-string-literal
: conversion of strings containing backslash characters to raw C++ strings -
modernize-use-nullptr
: use ofnullptr
instead ofNULL
-
modernize-use-override
: enforce the use of override; this also removes thevirtual
qualifier since in the presence ofoverride
orfinal
this is unnecessary.
To run linting on a local machine, one requires the following series of commands.
First, run the CI to configure CMake, install any third-party dependencies, export all compilation commands (which will be read by clang-tidy
) but stop short of compiling the code. This ensures that headers are all in place for clang-tidy
.
EXPORT_COMPILE_COMMANDS=1 BUILD_TYPE=full bash -x .gitlab-ci/build-and-test.sh
It is important to note that this tool will only look at files that will be compiled, thus why we need to run in full mode. This means:
- there might be some files that are still not picked up with some rather more specialised configuration options, so we should make sure that the CI is indeed compiling everything!
- there are some files I found and removed in !1427 (closed) that are not being compiled; however to keep it cleaner here I suggest removing these in a future MR.
Next, run clang-tidy
and tell it to apply all changes:
export CL_T_VER=14 # using clang-tidy-14 by default
run-clang-tidy-$CL_T_VER -p build -header-filter='(library|solver|tests)' \
-j$NUM_CPUS -fix \
-clang-tidy-binary=clang-tidy-$CL_T_VER \
-clang-apply-replacements-binary=clang-apply-replacements-$CL_T_VER
In the above:
-
clang-tidy
looks for thecompile_commands.json
within thebuild
directory specified by-p
-
-header-filter
tellsclang-tidy
where to look for headers for our code: otherwise it may try to apply changes to system headers, which isn't very useful -
-fix
tellsclang-tidy
to apply the fixes it finds.
Finally, formatting will need to be fixed with clang-format-11
as normal. I suggest keeping this, if possible, since in my testing, later version of clang-format
introduce some weird formatting bugs with our particular configuration.
Tests
A new CI template .clang-tidy-template
has been added which runs the above on the CI. If the resulting repository has any differences as detected with git diff
then these are kept as an artifact, so that changes can be very easily applied by downloading the diff and running git apply
.
Suggested reviewers
Notes
Please add any other information that could be useful for reviewers.
Checklist
-
Functions and classes, or changes to them, are documented. -
User guide/documentation is updated. -
Changelog is updated. -
Suitable tests added for new functionality. -
Contributed code is correctly formatted. (See the contributing guidelines). -
License added to any new files. -
No extraneous files have been added (e.g. compiler output or test data files).